- Notifications
You must be signed in to change notification settings - Fork1.1k
fix: remove defaults for CODER_EMAIL_ options#15482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
738dceb to19398ccCompareDanielleMaywood commentedNov 13, 2024
Ifcoder/serpent#23 gets merged (and released), this would fix the issue encountered here. |
dannykopping left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for this! A couple minor points but otherwise LGTM.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| Description:"The hostname identifying the SMTP server.", | ||
| Flag:"email-hello", | ||
| Env:"CODER_EMAIL_HELLO", | ||
| Default:"localhost", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we can keep this one. This will break existing installations if not explicitly defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm happy to do this although an issue with keeping this one is if someone has setCODER_NOTIFICATIONS_EMAIL_HELLO then we will be changing it back to "localhost" for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we wait untilcoder/serpent#23 is merged then that won't be the case, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I managed to solve the only open question I had forcoder/serpent#23 and I'm pretty happy with it. We've increased test coverage and all coder/coder tests are passing.
Basically just waiting for an approve, so do you want to go ahead with this PR@DanielleMaywood or do we go withcoder/serpent#23? I didn't review yet, but if you want this merged I can do a quick review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we go withcoder/serpent#23 first, as that is a prerequisite to get this to behave how we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
PS. Another neat change in 23 is that you can define the same default on both the current and the deprecated option (should help with docs), and if they diverge it'll be detected as an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks like you have two reviewers for that PR already, but do shout if need another 👍
Co-authored-by: Danny Kopping <danny@coder.com>
temporarily using new branch from coder/serpent for this
DanielleMaywood commentedNov 14, 2024
Closing as scope changed.#15476 has fixed the original issue here (by updating to a new version of coder/serpent). |
Fixes#15480
The defaults in
$CODER_EMAIL_SMARTHOST,$CODER_EMAIL_HELLO, and$CODER_EMAIL_FORCE_TLSoverride values set with the (now deprecated)$CODER_NOTIFICATIONS_EMAIL_SMARTHOST,$CODER_NOTIFICATIONS_EMAIL_HELLO, and$CODER_NOTIFICATIONS_EMAIL_FORCE_TLSenvironment variables.This fixes that by removing the defaults. The defaults are helpful for local dev setups but for any serious deployment these will likely be changed anyways so thisshouldn't cause any issues for customers existing deployments.