1030 - Refactor SMTP settings to be outside of passcode config #1121
Conversation
|
Hey @irby, the changes are looking good, but IMO it would be good if we can do it without breaking changes. My preferred solution would be, that the old smtp config is marked as deprecated and when someone uses it, that we log a deprecation message that the new smtp path should be used. |
|
Hi @FreddyDevelop sure I understand that. Im traveling for a conference today so I may have to address this sometime early next week. If you could, could you provide me with some rules around the config? Do you want me to override the values of root level SMTP config with passcode config if that level is filled out? Do you want to prefer root level SMTP config over passcode SMTP config if root level SMTP config is provided? |
Sorry @irby @FreddyDevelop I should have thought about that when we talked about it in the other thread. |
|
Hey @irby, sorry for the late response.
I would say we should use the root level SMTP config when it is filled otherwise use the passcode SMTP config. When both are set, the root level SMTP config should be used. |
|
Hey @irby any news? |
|
Hey @FlxMgdnz, haven't been able to touch this in some time to commitments at work keeping me busy. My schedule should be clearing up in the next couple weeks. |
Thanks @irby. Would be awesome if this gets resolved. Let us know as soon as you'll find the time. Someone from the team will happily pair up with you anytime. |
|
Hi @FlxMgdnz I have pushed a new commit that will allow for compatibility for SMTP settings defined in the Passcode section. The approach for this is as follows:
|
backend/config/config.go
Outdated
| @@ -638,6 +648,16 @@ func (c *Config) PostProcess() error { | |||
|
|
|||
| } | |||
|
|
|||
| func (c *Config) arrangeSmtpSettings() error { | |||
| if c.Smtp.Validate() == nil && c.Passcode.Smtp.Validate() == nil { | |||
| return fmt.Errorf("conflicting smtp settings found, please remove either smtp or passcode.smtp from the config") | |||
There was a problem hiding this comment.
Please don't return an error here, instead log a warning or ignore this case. IMO there is no benefit of returning an error.
There was a problem hiding this comment.
In this case, would you want the root level SMTP to override passcode SMTP settings?
I'm fine with logging a warning here, I think we should let users know the config is potentially conflicting.
There was a problem hiding this comment.
Yes, in this case the root level smtp config should be used.
Yeah, would be good to let users know that there is a conflict in the config.
There was a problem hiding this comment.
Is there a preferred way to format a warning message in this project? Do you use fmt to log these?
There was a problem hiding this comment.
No, we have zerolog in place and log in json as format.
There was a problem hiding this comment.
@FreddyDevelop Updated per feedback.
backend/config/config.go
Outdated
| func (c *Config) arrangeSmtpSettings() { | ||
| if c.Passcode.Smtp.Validate() == nil { | ||
| if c.Smtp.Validate() == nil { | ||
| zeroLogger.Warn().Msg("Both smtp and passcode.smtp are set. Using smtp settings from passcode.smtp") |
There was a problem hiding this comment.
This warning message is wrong, the root smtp settings will be used.
There was a problem hiding this comment.
Ah, thank you. Updated.
…anko#1121) * 1030 - Refactor SMTP settings to be outside of passcode config * Backwards compatibility of SMTP settings * Do not error if root smtp and passcode smtp are defined. Log warning instead * Update warning message
Description
As discussed in #1030 , when security notifications are introduced, SMTP settings would no longer be strictly applicable to passcodes. I propose we break SMTP settings into its own section in the configuration file.
Implementation
In this PR, I have moved SMTP up to the base level of the configuration and reflected this change in all config yaml files
Additional refactors:
.nvmrcfile in e2e to Node value described in its README