Skip to content

Conversation

@nightah
Copy link
Contributor

@nightah nightah commented Jun 18, 2021

As it currently stands all notifiers utilise os.Hostname to populate their titles/subjects.

When utilising Docker with a bridged network if you set the hostname for a container to an external DNS hostname Docker's internal DNS resolver will override said hostname for all containers within the bridged network.

This change allows a user to specify what hostname should be represented in the notifications without having to change the os.Hostname.

As it currently stands all notifiers utilise `os.Hostname` to populate their titles/subjects.

When utilising Docker with a bridged network if you set the hostname for a container to an external DNS hostname Docker's internal DNS resolver will override said hostname for all containers within the bridged network.

This change allows a user to specify what hostname should be represented in the email notifications without having to change the `os.Hostname`.
@nightah nightah requested a review from piksel as a code owner June 18, 2021 05:28
@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #994 (d1d6aa8) into main (f508c92) will increase coverage by 0.06%.
The diff coverage is 76.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #994      +/-   ##
==========================================
+ Coverage   56.15%   56.22%   +0.06%     
==========================================
  Files          25       25              
  Lines        1453     1462       +9     
==========================================
+ Hits          816      822       +6     
- Misses        567      568       +1     
- Partials       70       72       +2     
Impacted Files Coverage Δ
pkg/notifications/notifier.go 32.81% <33.33%> (-0.53%) ⬇️
internal/flags/flags.go 88.65% <100.00%> (+0.17%) ⬆️
pkg/notifications/email.go 90.90% <100.00%> (ø)
pkg/notifications/gotify.go 67.74% <100.00%> (ø)
pkg/notifications/msteams.go 68.42% <100.00%> (ø)
pkg/notifications/slack.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f508c92...d1d6aa8. Read the comment docs.

Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The title isn't tied to email anymore (hence why it's in pkg/notifications/notifier.go). It should not be too difficult to set the notification hostname either to the flag value, or if empty, os.Hostname.

There are more places to change than the one below obviously though.

@nightah nightah changed the title feat: allow hostname override for email notifier feat: allow hostname override for notifiers Jun 18, 2021
Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was fast!

The way it's implemented here is probably the simplest (in a good way). A larger overhaul of the notification flag handling should probably be done at a later stage anyway.

Just one comment:

@simskij simskij merged commit dc12a1a into containrrr:main Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants