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

helm add email config #780

Merged
merged 6 commits into from
Mar 26, 2022
Merged

helm add email config #780

merged 6 commits into from
Mar 26, 2022

Conversation

lusson-luo
Copy link

@lusson-luo lusson-luo commented Mar 25, 2022

Description

helm provides email smtp-secret,easy help people use email notification
Only helm configuration changes are included

Checklist

  • I have reviewed the contributing guidelines.
  • I have included unit tests for my changes or additions.
  • I have successfully run make test-docker with my changes.
  • I have manually tested all relevant modes of the change in this PR.
  • [] I have updated the documentation.
  • I have updated the changelog.

Questions or Comments

@jertel
Copy link
Owner

jertel commented Mar 25, 2022

What's the benefit of using this vs the suggested method of an extra volume mount (commented out at the bottom of values.yaml)? While this would work it means storing credentials in the values.yaml, vs the suggested method of creating the credentials as a secret separately from the chart's general config.

If we proceed with this PR then the documentation needs to be updated in README.md, and an explanation should be included that notes the difference between using this method vs the other method. Also the contributing guidelines must be reviewed and the changelog needs to be updated.

@lusson-luo
Copy link
Author

It is the practice of the multi-volume mount method. Before smtp-auth Secret was created manually.
This commit is a configuration that automatically creates a secret based on the values configuration and uses it with mount.
After this pr, users use email alerts instead of manually creating secrets themselves, just release the annotations

@lusson-luo
Copy link
Author

Do you think this pr will continue?

@jertel
Copy link
Owner

jertel commented Mar 25, 2022

My personal belief is that secrets should not be stored in chart config files, but rather should be managed separately. However, if you (and others) prefer to mix config data with secrets, that is your choice. We can proceed with the PR but you will need to finish the contribution guideline requirements. Specifically:

  • Review and agree to the contribution guidelines
  • Add the documentation for the new config parameters to chart/elastalert2/README.md
  • Update the CHANGELOG.md with this change, following the existing patterns used in that file.

@lusson-luo
Copy link
Author

ok, let me continue with the following

@lusson-luo
Copy link
Author

ok, i have done the above

jertel
jertel previously approved these changes Mar 26, 2022
Copy link
Owner

@jertel jertel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I changed the principal key name to smtp_auth.username, for consistency with other fields. Thanks for your contribution.

@jertel jertel merged commit 5df69a8 into jertel:master Mar 26, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants