- Notifications
You must be signed in to change notification settings - Fork918
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 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
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.
Uh oh!
There was an error while loading.Please reload this page.
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 outSMTP from it would be a larger refactor and at the moment we only useSMTP 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.
823a2ea intomainUh oh!
There was an error while loading.Please reload this page.
Closes#14644