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

Skip helm get hooks when --three-way-merge=true #423

Merged

Conversation

stanislav-zaprudskiy
Copy link
Contributor

@stanislav-zaprudskiy stanislav-zaprudskiy commented Dec 1, 2022

Hooks manifests would have been already added when --three-way-merge=true (by genManifest function). Unless --no-hooks=true have been supplied as well - making them being skipped earlier as well.

This results into only one case when hooks manifests should be added separately: --three-way-merge=false --no-hooks=false (the current defaults).

With the current code however, when charts contain hooks and --three-way-merge=true, some confusing output is returned (esp. on empty diff). That's because hooks manifests are added twice: first inside genManifest function, and then again by appending results of getHooks function.

$ helm diff upgrade my-release my-chart --values values.yaml --three-way-merge=true
2022/12/01 16:52:34 Error: Found duplicate key "my-ns, my-cm, ConfigMap (v1)" in manifest

The exit code is 0, though. A user thinking could be:

  • Did helm diff fail, but did not reflect that in exit code?
  • It did not fail, but because of the Error it could have missed diffing at all?
  • Why the diff is empty - because of the Error or is it empty indeed?

Perhaps, replacing Error with Warning in the message would make sense as well.

Verified

This commit was signed with the committer’s verified signature.
XSAM Sam Xie
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.

@stanislav-zaprudskiy Thank you very much for the fix, along with your detailed report! This looks awesome! Merging.

@mumoshu mumoshu merged commit 609de73 into databus23:master Apr 23, 2023
hexcsl pushed a commit to hexcsl/helm-diff that referenced this pull request Jul 3, 2023

Verified

This commit was signed with the committer’s verified signature.
XSAM Sam Xie
hexcsl pushed a commit to hexcsl/helm-diff that referenced this pull request Jul 3, 2023
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.

None yet

3 participants