Skip to content
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

fix: change default setting of SMTP_SSL #545

Merged
merged 4 commits into from
Mar 26, 2024

Conversation

danone-dev
Copy link
Contributor

This fixes #537
Changed the default of SMTP_SSL to 'not set'. Also update the redmine_configure_email_delivery function so it removes the ssl: line from configuration.yml if no SMTP_SSL environment variable is set.

If you need SMTP_SSL: true you would have to set the variable anyway, even in the old situation.

Changed the defaukt of SMTP_SSL to not set. Now the ssl: setting in configuration.yml is only present if SMTP_SSL is explicitly set.
@jcormier jcormier self-requested a review February 22, 2024 14:36
@jcormier jcormier self-assigned this Feb 22, 2024
@jcormier
Copy link
Collaborator

I gather ssl: false is not a valid setting then? You don't outright say it but it seems implied by your change.

If that's the case, then I'd prefer we invert this check and delete this setting if it's set to anything but true. That way if someone sets the ENV variable to false, it still works correctly.

if [[ "${SMTP_SSL}" != "true" ]]; then

@danone-dev
Copy link
Contributor Author

The only thing that has changed now is that SMTP_SSL: true will end up in the config, and SMTP_SSL: false will be removed from the config. The default remains SMTP_SSL: false, so the README and env-defaults don't have to be modified.

@jcormier jcormier merged commit f3dc70d into sameersbn:master Mar 26, 2024
1 check passed
@jcormier
Copy link
Collaborator

Do you need this backported to a previous release?

@danone-dev
Copy link
Contributor Author

No, this is fine for me. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An error occurred while sending mail
2 participants