New chart diffing

[jrnt30]

This provides the ability to do a diff on a release that does not currently exist. This makes it much easier to use the plugin with things like helmfile as it doesn't error out and provides valuable feedback.

NOTE: I have an additional PR that manages the merge conflicts of my two PRs that might be easier to merge for you.

[databus23]

Hmm. I'm not sure about this one. Specifically that it does not fail with an error if the release you specified on the command line does not exist. So if you have a typo in the release name you won't get an error. If you using helm diff in a script that might lead to unexpected behaviour.

[databus23]

Sorry that it took me so long to respond. I'm just thinking maybe its better to keep the focus of this plugin on diffing stuff. What you are trying to accomplish sound to me like it could be done with a wrapper script that decided if its an install or an upgrade and either does helm install ... --dry-run --debug or helm diff. I have to admit though I don't fully understand your use case.

[jrnt30]

Thanks for the reply. You're absolutely correct around the mistype thing and if an error is a critical component of the workflow, I could add in a feature flag to enable this functionality.

For our use case specifically, we use two tools in conjunction with one another to analyze how "far out of compliance" a given environment is via our CI/CD system. We use the helmfile tool to create a manifest of everything that should be present. We then run helmfile diff to provide a listing of all changes present in the system which, depending on the environment, may be a step in our workflow that then waits for an operator to review and give the 👍 before it proceeds to apply those changes via helmfile sync.

The problem we are experiencing is the times we add a new release to an environment the helmfile diff will fail, due to the underlying diff plugin failure. For us, a more accurate representation of our needs is to actually display the "real diff" if we were to actually run helmfile, as this does the helm upgrade --install under the covers.

[jrnt30]

@databus23 Does that clarify for you how we prefer to use the tool? Would you like me to throw this behind a feature flag or is it simply something you do not intend to support?

Thanks!

[foklepoint]

This would be really great to have. @databus23 we have the exact same use-case as @jrnt30. Can we please get this merged, even behind a feature flag, this would be really really great to have

[gtaylor]

Hate to pile on, but this would definitely be useful!

[foklepoint]

This is a little out of date and doesn't compile in its current state. This is going to need to fix the spec type from map[string]string -> map[string]*manifest.MappingResult on line 96 here: https://github.com/databus23/helm-diff/pull/15/files#diff-7ddfb3e035b42cd70649cc33393fe32cR96

[mumoshu]

@jrnt30 @foklepoint Just asking to explore the possibility - but would it be ok if helmfile conditionally run either diff helm install ... --dry-run --debug(as suggested by @databus23) or helm diff? Something like running the former if the release doesn't exist yet (i.e. if ! helm status $release; then ...).

A concern is that, outputs from the former command won't be real diffs so we probably need to leverage aryann/difflib and mgtuz/ansi as helm-diff does.

[mumoshu]

Anyway, from helmfile's perspective, this feature hidden behind a flag like -N --new-release would be preferable.

Btw, FWIW, the vanilla diff command actually has a similar flag with a similar behavior to this feature

$ diff --help | grep new-file
  -N  --new-file  Treat absent files as empty.
  --unidirectional-new-file  Treat absent first files as empty.

So if I rush(!) from a helmfile dev's perspective, I can say that people may expect a diff-like command to have a flag to allow diffing against an empty target.

[jrnt30]

I will take a look at this again. While it would be possible to conditionally check for the existence of the release before calling diff vs install it simply increased complexity and still wouldn’t address the non-helmfile workflow. I will update the code this weekend and throw it behind a feature flag, that seems like a very reasonable compromise that will provide us some immediate value - JRN

…

On Apr 11, 2018, at 9:11 PM, KUOKA Yusuke ***@***.***> wrote: Anyway, from helmfile's perspective, this feature hidden behind a flag like -N --new-release would be preferable. Btw, FWIW, the vanilla diff command actually has a similar flag with a similar behavior to this feature $ diff --help | grep new-file -N --new-file Treat absent files as empty. --unidirectional-new-file Treat absent first files as empty. So if I rush(!) from a helmfile dev's perspective, I can say that people may expect a diff-like command to have a flag to allow diffing against an empty target. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[jrnt30]

Merge conflicts resolved and gated the new command behind the --allow-unreleased flag. I am happy to change the name of the flag if someone has a more succinct name.

[mumoshu]

@databus23 Hi, thanks for maintaining helm-diff! Excuse me but would this feature make sense to be implemented within this project?

I'm continuously receiving relevant feature requests like this to helmfile and am trying to decide where to implement this.
FWIW, I can help maintaining this feature if this gets merged to helm-diff.

[rendhalver]

We use helmfile and helm-diff with Jenkins.
We run helmfille diff when a PR is tested and helmfile sync when it is merged.
So for deployments that don't already exist out PR's fail because there is nothing to diff against.
This would also be handy for us as well.
Let me know if you need help testing.

[jeff-knurek]

@databus23 can you provide some opinion on this topic? then maybe this PR or #36 or some other change can be made to close this issue

[foklepoint]

This feature has become very important to us. It's kind of silly that we haven't had any progress on this issue which can be so critical for helm-diff in the CI environment since December 5th (Almost 6 months soon).

[mumoshu]

Hi @jrnt30! Would you mind rebasing this? Please feel free to ask me if you don't have bandwidth to do so.

Long story short, I talked with @databus23 and I'm one of maintainers now. After some discussion and thuoghts, I still believe that this change makes sense to be within helm-diff.

---

During the discussion @databus23 and I had, the concern was how we can keep helm-diff simple and clean. For this particular change, the point was why something like if is_new; then helm install --debug --dry-run; else helm diff; end; wasn't enough.

My answer, which I suppose not complete, is that we can:

• Add better error handling
• Add better messaging
• Better wrap helm invocations

These are advantages over the vanilla helm and bash snippets.

Also we have consensus that considering similarity to the unix diff tool would be a good thing.

That being said, I believe this change makes sense to be within helm-diff.

[jrnt30]

In addition to your points, there are several tools that rely on helm in the background (landscaper, helmfile, etc.) where injecting arbitrary logic around is_new is not really an option.

I can make the updates again, is the desire to keep this behind a feature flag then or to have it be the normal activity?

[mumoshu]

Thanks!

where injecting arbitrary logic around is_new is not really an option.

I'm just trying to understand. Are you saying that we will be forced to write one-off bash snippet like that for each helm-based tool you use(landscaper, helmfile, etc.), correct?

I can make the updates again, is the desire to keep this behind a feature flag then or to have it be the normal activity?

I slightly prefer it behind the flag for backward-compatibility.

For maximum simplicity making it as a default behavior would be nice. But I'm not exactly sure if it justifies possibility to the break compatibility and the consensus 🤔

[mumoshu]

If the project was named helm-plan, I'd make it a default behavior after terraform plan :) That's another story though.

[mumoshu]

Let's go ahead. Thanks for your patience and the contribution @jrnt30, and your interest and the discussion everyone!

[jeff-knurek]

👏
it'd be nice to have a new release tag: https://github.com/databus23/helm-diff/releases

[rendhalver]

I will second that.
A new release would be awesome.