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

[bitnami/common] Allow handling of new secrets after initial installation #28581

Merged

Conversation

cwrau
Copy link
Contributor

@cwrau cwrau commented Jul 30, 2024

Description of the change

This changes the handling of non-existing secrets that should be generated so they will be generated if failOnNew is true.

Benefits

This allows the user to add new secrets to a managed secret without having to specify it and letting this chart generate it.

Possible drawbacks

Not that I can think of.

Applicable issues

Non that I created, but we sometimes do https://github.com/teutonet/teutonet-helm-charts/blob/main/charts/base-cluster/templates/monitoring/kube-prometheus-stack/oauth-proxy-secret.yaml?plain=1#L14C204-L14C356 to work around this problem.

Additional information

Checklist

  • Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
  • Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
  • Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
  • All commits signed off and in agreement of Developer Certificate of Origin (DCO)

…tallation

Signed-off-by: Chris Werner Rau <cwrau@cwrau.info>
@cwrau cwrau force-pushed the feat/common/improve-new-secrets-handling branch from fbd1f78 to 00b8c29 Compare July 30, 2024 13:24
Signed-off-by: Bitnami Containers <bitnami-bot@vmware.com>
@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels Jul 31, 2024
@github-actions github-actions bot removed the request for review from carrodher July 31, 2024 07:10
@github-actions github-actions bot removed the triage Triage is needed label Jul 31, 2024
@github-actions github-actions bot requested a review from juan131 July 31, 2024 07:10
Signed-off-by: Bitnami Containers <bitnami-bot@vmware.com>
@juan131
Copy link
Contributor

juan131 commented Jul 31, 2024

Hi @cwrau

I'm sorry but the changes view that GH offers it's not the best in this case and I'm unable to fully understand the changes... Could you do a quick summary of the behavior before & after your changes? Thanks in advance.

@cwrau
Copy link
Contributor Author

cwrau commented Aug 1, 2024

Hi @cwrau

I'm sorry but the changes view that GH offers it's not the best in this case and I'm unable to fully understand the changes... Could you do a quick summary of the behavior before & after your changes? Thanks in advance.

Currently, when you add a secret to be generated after the chart was initially installed, the upgrade fails saying that the value has to be supplied, even if failOnNew is false. To be honest, I don't know what that flag is for, even when I happened to have a scenario where the corresponding logic hit, it would result in an empty secret, which is not desirable.

This changes this logic in the case of failOnNew, to not fail when a secret get's added.

Copy link
Contributor

@juan131 juan131 left a comment

Choose a reason for hiding this comment

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

Thanks for providing the details! I've been reviewing the changes and it seems your approach is more robust and streamlined.

In any case, I'll ask for a second review before approving it.

@juan131 juan131 merged commit 07062ee into bitnami:main Aug 5, 2024
11 checks passed
@javsalgar
Copy link
Contributor

javsalgar commented Aug 9, 2024

Is this issue related with this change? #28770

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants