gissilabs / charts

Apache License 2.0
37 stars 22 forks source link

Implement new SMTP SSL env variable and upgrade base image #36

Closed lord-kyron closed 1 year ago

lord-kyron commented 2 years ago

Hi, I've made some changes to implement the latest SMTP options to the helm chart + bump the image to the latest version, as I've explained here: https://github.com/gissilabs/charts/issues/35

@sgissi - please, review the changes! Thanks!

lord-kyron commented 2 years ago

@sgissi can you please, check and merge this if it is ok? It will help using the implementations for the newest version of Vaultwarden image.

sgissi commented 2 years ago

Hi @lord-kyron, thanks a lot for the PR! That would break backwards compatibility, I would prefer to deprecate the settings: add a warning at NOTES.txt, a notice at README and prefer smtp.security over smtp.ssl/explicittls. But I wouldn't like to break existing installations of the chart, particularly when while upstream still supports the old env variables.

Do you know the first version of Vaultwarden that supports SSL_SECURITY?

*Edit: I'm so sorry this went a month undetected! I had a house move during that period and I didn't catch the notification.

lord-kyron commented 2 years ago

Hi @sgissi, Yes here is the official statement:

From v1.25.0, environment variable for SMTP SSL/TLS configuration has been updated to SMTP_SECURITY (which was mislabelled, see bug #851).
sgissi commented 2 years ago

Thank you for the update. It looks like the option was deprecated fully upstream (https://github.com/dani-garcia/vaultwarden/commit/42136a70973f60086749c62439c6a965d4589c02) so upgrading will be a breaking change in any case. My recommendation for this PR is:

lord-kyron commented 2 years ago

@sgissi - can you please, take a look what I've done to the pull request, according to your recommendations (please, check the deployments.yaml template as I am not sure about the 'if semVer' statements and syntax/indent are right). Thanks!

jonkerj commented 2 years ago

Any news? I'm using this chart, and not having this PR prevents me from updating Vaultwarden

lord-kyron commented 2 years ago

@sgissi please

sgissi commented 1 year ago

Thanks for the updates, life got really busy lately :) I'll review them this weekend. In the meantime, you can use the "extraEnv" to supply the right values to Vaultwarden until the chart catches up.

jonkerj commented 1 year ago

Any chance you can look at it?

lord-kyron commented 1 year ago

@sgissi it will be great if you can check what I improved and merge it!

benedikt-bartscher commented 1 year ago

@sgissi, new year, new luck? :)

lord-kyron commented 1 year ago

@sgissi Happy New year :) Can you please check my latest changes to this pull request? We really need the changes for this alraedy.

sgissi commented 1 year ago

Hi @lord-kyron, @benedikt-bartscher and @jonkerj. Thanks again for the PR updates and sorry for the delayed review. I'll merge these to a branch and work out some more updates to Vaultwarden 1.27 before merging to master and releasing new charts.

lord-kyron commented 1 year ago

Thank you @sgissi !