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

Don't set default caBundle for webhooks #617

Merged
merged 2 commits into from
Jun 9, 2021
Merged

Don't set default caBundle for webhooks #617

merged 2 commits into from
Jun 9, 2021

Conversation

jstewart612
Copy link
Contributor

Fixes #614

@jstewart612
Copy link
Contributor Author

@mumoshu Helm chart syncing functionalities do three-way merges of generated YAML. When you specify a value for caBundle, it clobbers the value that the controller puts in there after it's setup. This makes CD systems like ArgoCD and Flux unable to keep the Helm chart in sync because it enters a constant update loop for this value between rendered chart and the on-cluster value which the controller is setting. This breaks the loop.

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 9, 2021

@jstewart612 Thanks for the PR! This looks good, except Helm 2. I know Helm 2 is already deprecated and perhaps not maintained anymore, but I want to prepare before being asked. That said..

Helm chart syncing functionalities do three-way merges of generated YAML

What would happen when you run helm upgrade on the updated chart with helm 2, which doesn't support three-way merge? Override the caBundle field with an empty value?

@jstewart612
Copy link
Contributor Author

Definitely not maintained anymore: https://helm.sh/blog/helm-v2-deprecation-timeline/

I don't know what happens, but feel it more than sufficient to no longer support that which is deprecated.

Copy link
Collaborator

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix @jstewart612 ☺️

@mumoshu mumoshu merged commit 8566a4f into actions:master Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants