-
Notifications
You must be signed in to change notification settings - Fork 934
chore(cli): drop 'notification' prefix for configuring email auth #15270
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
LGTM, with just one question. Will leave it to someone who's been here a bit longer to actually approve for now.
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.
Just a minor nit about struct field name, otherwise LGTM 👍🏻
hello: localhost | ||
# Force a TLS connection to the configured SMTP smarthost. | ||
# (default: false, type: bool) | ||
# (default: <unset>, type: bool) |
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.
We should probably start adding the deprecation notice to these comments at some point 🤔
Description: "The sender's address to use.", | ||
Flag: "email-from", | ||
Env: "CODER_EMAIL_FROM", | ||
Value: &c.Notifications.SMTP.From, |
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.
Why are we keeping .Notifications
here? Rename to email?
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 kept .Notifications
for now because separating out SMTP
from it would be a larger refactor and at the moment we only use SMTP
for notifications so didn't think it warranted being put into this PR.
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.
Alright, fair enough 👍🏻. Not a pressing issue but worth doing at some point to match the configuration.
Closes #14644