-
Notifications
You must be signed in to change notification settings - Fork 279
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
Show diff for actual state vs desired state #176
Comments
That would be nice but its not straightforward. The problem is that Kubernetes controllers/operators add a lot of fields when working with the resources. For example the common |
@databus23 what about using kubectl-diff behind the scenes after rendering the templates? |
Not sure I follow you. What do you mean by that? Against what would you diff the rendered templates? The actual state from the cluster is littered with dozens of default values that are not in the rendered templates. This would in almost all cases create a huge diff that has no meaning. |
What I mean is that when running helm-diff the following will happen:
Example of output which includes the diff from the actual state of the clustered:
|
I tested this using
The results were to show only 2 differences:
Seems to me that this suggestion could be a viable solution. (btw - thank you for this package!) |
helm/helm-www#521 <-- I filed this in the wrong place, guess you already know about this :) |
For what its worth, I tried what @corradomatt mentioned above, but had pages of output, likely due to everything not being in one namespace. Apparently, to do this I would have to separate the comparison by namespace (somehow?). |
I've been using the following to run diffs now... # using helmfile
helmfile -e myenv template | kubectl diff -f - | bat -l diff -
# using helm directly
helm template releaseName chart | kubectl diff -f - | bat -l diff - You could easily apply your namespace flags to these commands and/or supply a selector to helmfile to limit the output. This also uses the bat utility (which is amazing btw). |
On this note now that helm 3 is doing a 3 way merge could we not look at the code they are using for the merge. |
I would just like to also mention the importance of having this feature 🙂 Before, with Helm2, showing a diff excluding manual changes in the cluster was fine because Helm2 would also ignore these. But now, with Helm3 and the new 3-way merge, we can't really be sure of what will be applied to the cluster when using this plugin. If the upgrade is going to revert some manual changes then this plugin won't show them. |
Agreed this is much more important now. It is also needed to help debug why a 3 way merges in helm maybe not acting as we expect. |
To support helm/helm#7649, it'd be handy if the "actual state" diff side in this feature could be one not provided by Helm. Obviously, in such a case, only objects created by the Helm release would be considered, although it might be handy to explicitly flag objects that exist but are missing the appropriate annotations (although they'd show up in the diff anyway). |
@mumoshu any thoughts on this issue? Is it hard to fix? Or impossible? |
Same issue is impacting helmsman (and i believe helmfile) which used to use helm-diff plugin to decide if a "helm upgrade" will trigger any changes or not. And if helmsman doesnt see any changes to be applied it just skips the "helm upgrade" step. But in case of any out-of-helm changes are involved, it becomes blind due to the fact that helm-diff calculates the diff against "latest deployed version of a release". You can see some more context in Praqma/helmsman#487 (comment) Here the issue is not how helmsman or helmfile uses helm-diff, its actually the users' expectation when they are using the helm-diff plugin. Most of the cases they do care about "which actual changes to be applied", rather than "what would be the change if there has been no manual changes in the cluster". But either may be confusing for the helm-diff users, so maybe introducing a new flag and providing both functionality is the best path forward. |
I've just looked into helm diff command usage a bit.
So here is a sample case that Prepare a test environment
Now, here is what
And now, lets see if that's true (helm diff reported me a misleading update result):
|
@bergerx I believe everyone here is already in agreement about the behaviour of |
Ah, thanks for the clarification, so we just need a volunteer to work on the PR. |
@sstarcher @max-rocket-internet Finally I could read some code related to how three-way merge patch is done in helm3: https://github.com/helm/helm/pull/6124/files but I'm not yet sure how we could leverage it. Maybe we need to add a some kind of |
@dudicoco @corradomatt Thanks a lot for suggesting the use of kubectl-diff! It does seem to work in your examples, but I'm not entirely sure if there's any possible edge-case. If My thought may distill into the same concern @databus23 has expressed earlier. |
I think the easiest path would be to send a pull request to the upstream as described in #176 (comment), and just use that instead of helm-diff. helm-diff will remain as the way to show diff between the current release and the new release, but tools like helmsman and helmfile should have an option or(sane default) to let the tool use the new helm command, so that the existence of the generated three-way merge patch can be used to detect if there's any "real" change to be applied or not. |
Should I create an issue in the Helm repo for this? |
@max-rocket-internet That would be super helpful for all of us 👍 |
I find the maintainers of helm to be terse at times but let's see how it goes: helm/helm#8718 |
did anyone had a look at https://github.com/bacongobbler/helm-patchdiff from helm/helm#8718 (comment) ? |
Unfortunately I only now found out that this issue exists.
|
Any updates on this? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Bump |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Bump |
Please, any updates on this? |
@mumoshu thanks for your work here. I just have one question after testing with latest helm-diff release though: are annotations ignored? I can install a random chart:
Make a manual change:
And helm-diff shows the change now 🎉
...
- -config
- /home/toxiproxy/proxies/config.json
- image: shopify/toxiproxy:2.1.3
+ image: shopify/toxiproxy:2.1.4
imagePullPolicy: IfNotPresent
livenessProbe:
failureThreshold: 2
... But if I add a manual annotation:
It is not shown in the diff output 🤔 |
@max-rocket-internet I think that's intended behavior / that's how three-way merge works. Annotations will be merged by keys among the current state, the latest manifest stored in the helm release, and the desired manifests rendered. |
OK yes, you are right. Thanks again @mumoshu 💖 |
Was there ever any actual practical solution or workaround to this? |
@jimbali |
thanks @chadlwilson |
Still doesn't seem to pick up manual changes for me though! 🤷♂️ |
If you think something isn't working perhaps it is best to open a separate issue with your scenario. I find folks are sometimes confused with how Helm three-way merge works as in the comment above. #176 (comment) If Helm 3 is not going to revert your manual change on apply (which often it will not), it shouldn't show in the output of helm diff. At least if working as I understand was intended. Are you sure your manual change is something Helm is going to revert if applied? |
I'd have thought that it would do, as the liveness and readiness probes were deleted manually, and they are explicitly present in the chart. |
Test it with Helm. I think you will find it will not reinstate such manual changes with Helm 3s default 3 way merge behavior. |
Hi,
At the moment, if you make any manual changes to resources (not via helm) helm diff will not reflect these changes.
I suggest that the output should reflect the desired vs actual state of the resources, just like running terraform plan.
The text was updated successfully, but these errors were encountered: