fix: remove defaults for CODER_EMAIL_ options #15482
Conversation
738dceb to
19398cc
Compare
|
If coder/serpent#23 gets merged (and released), this would fix the issue encountered here. |
codersdk/deployment.go
Outdated
| @@ -1038,7 +1037,6 @@ when required by your organization's security policy.`, | |||
| Description: "The hostname identifying the SMTP server.", | |||
| Flag: "email-hello", | |||
| Env: "CODER_EMAIL_HELLO", | |||
| Default: "localhost", | |||
There was a problem hiding this comment.
I think we can keep this one. This will break existing installations if not explicitly defined.
There was a problem hiding this comment.
I'm happy to do this although an issue with keeping this one is if someone has set CODER_NOTIFICATIONS_EMAIL_HELLO then we will be changing it back to "localhost" for them.
There was a problem hiding this comment.
If we wait until coder/serpent#23 is merged then that won't be the case, right?
There was a problem hiding this comment.
I managed to solve the only open question I had for coder/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 with coder/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.
I think we go with coder/serpent#23 first, as that is a prerequisite to get this to behave how we want.
There was a problem hiding this comment.
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.
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
|
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 this shouldn't cause any issues for customers existing deployments.