Allow chart versioning

[foklepoint]

We rename the existing version flag
and re-use the existing helm version flag.
This allow helm-diff to seamlessly work with
other helm tooling such as helmfile

[foklepoint]

@databus23 can we get this merged? thoughts? We've been using this in production at reddit for a while now and it works fairly well.

[sstarcher]

@databus23 this is a ticket that the https://github.com/roboll/helmfile project is waiting on

[databus23]

This looks like a duplicate of #1

[sstarcher]

@databus23 this is a PR that implements #1

[databus23]

Sorry I meant duplicate of #3

[sstarcher]

Looks like #3 also implements this, but in a different way

[sstarcher]

@databus23 I would think the @version would be more confusing than --diff-version version

[databus23]

I'm fine with adding this feature in general. I don't like repurposing existing flags. Does it have to be -v or was that just so that it looks nice? What about --chart-version?

What I like about the @version suffix in #3 is that it is part of the chart name. Specifying a flag like --chart-version doesn't make sense when [CHART] happens to be a local directory .

[sstarcher]

I don't think using -v or --version would make sense as that tends to imply the version of the plugin itself or verbose.

So from reading the other issues it looks like you have 2 requests.

• Diffs between versions
• Diffs between the current release and a new chart

Current syntax helm diff RELEASE CHART. If I'm following the code correctly CHART can be a local chart or a chart in a helm repository. The version syntax makes no sense for a local chart.

If you are willing to break the current syntax I think it would make sense to differentiate these things.

helm diff RELEASE CHART for a local chart when CHART exists as a local file
helm diff RELEASE@REVISION-REVISION2 for comparing 2 revisions
helm diff RELEASE CHART@VERSION for comparing the current release to a versioned chart

vs

helm diff RELEASE CHART
helm diff RELEASE -r REVISION-REVISION2
helm diff RELEASE CHART -d VERSION

Either syntax should work, but I think the second syntax would be easier to deal with in code.

[sstarcher]

@foklepoint @benlangfeld @mumoshu any opinions on the syntax

[benlangfeld]

What would the long-form versions of your proposed -r and -d options be, @sstarcher ?

[sstarcher]

--release and --diff-version I'm fine with different names. We could also go with --chart-version and use -c

[benlangfeld]

I'm happy with that, but you guys know best.

[sstarcher]

@benlangfeld ya, just trying to have the discussion so we can get some PR in to resolve this issue as this feature is much needed for anyone using helmfile.

[foklepoint]

So the idea behind reusing —version and -v is that helm uses those flags to indicate chart versions. As horrible as this is, helm makes its plugins a sub command of helm even though it is a separate program all together. As a sub-command, we should respect the flags of the parent program and work as seamlessly as possible with it. I understand that it might make more sense to go down the route of using chart@version and all, but I think that’s something we should discuss with helm upstream first.

Regarding your point about version with a local flag, i totally agree it doesn’t make sense to have versions on local flags, but this is currently a weird issue with helm itself. At the very least we stay as close to helms functionality as possible, even though an improved syntax might help in this case.

Lastly, programs like helmfile allow diff functionality through the use of this plugin. Helmfile also simply passes down the -v flag with the expectation that the plugin respects helm’s flags. If find ourselves moving away from this syntax, some minor coordination with other plugins in this eco system would be greatly appreciated.

All in all I think as a sub-command, we should be respecting the parent programs flags as much as possible.

[sstarcher]

@foklepoint I would be fine with that. If that's the case we should have a helm diff version command that display the version of the application. Which would align us with what helm version does and helm s3 version also.

If implemented that way it would leave the diffing between releases to some other mechanism.

@databus23 opinions

[foklepoint]

Yep that’s fair. I originally tried going that way but couldn’t get the flag/subcommand parsing to work easily so gave up :x

[sstarcher]

So if we keep @foklepoint direction we could have

helm diff RELEASE CHART # Diff the latest chart version
helm diff RELEASE -r REVISION-REVISION2 # Diff between releases
helm diff RELEASE CHART --version VERSION # Diff a new chart version

[mumoshu]

@sstarcher Looks awesome to me. And a helm diff version sub-command to replace the existing helm diff --version functionality in a same PR?

[mumoshu]

@databus23 Do you worry about the backward compatibility of the --version flag? 😉
If so, we still need to named it e.g. --chart-version as you've suggested.

So, it seems like:

helm diff RELEASE CHART # Diff the latest chart version
helm diff RELEASE -r REVISION-REVISION2 # Diff between releases
helm diff RELEASE CHART --version VERSION # Diff a new chart version
helm diff version # Show the version number

vs

helm diff RELEASE CHART # Diff the latest chart version
helm diff RELEASE -r REVISION-REVISION2 # Diff between releases
helm diff RELEASE CHART --chart-version VERSION # Diff a new chart version
helm diff --version # Show the version number

[databus23]

Ok, after some thought I'm now actually for using --version to specify the version of a chart.
The main reason for this being that this is how helm upgrade works and its nice to keep helm diff flag compatible.
It allows to "prepare" an upgrade using helm diff und when the output of helm diff looks good all that is left to do is to change helm diff to helm upgrade to apply the changes. At least this is how I have used helm diff in the past when performing upgrades manually.

Here is my proposal:

• Remove -v and add a helm diff version subcommand (same way helm works)
• Add --version to specify the chart version
• Add a helm diff chart RELEASE CHART subcommand
• Alias helm diff to helm diff chart
• (Not in this PR: Add helm diff revisions RELEASE [FROM] [TO] command)

I think without adding subcommands it will be difficult to add support for diffing revisions which has been requested a few times by now. Aliasing the root command to helm diff chart should provide backward compatibility (unless your release is called "version" or "revisions" 😄 )

Any comments on that?

[sstarcher]

@databus23 I think that all makes great sense. @foklepoint would you be able to implement the helm diff chart and helm diff version in this? I imagine we can leave helm diff revisions to a separate PR.

[databus23]

I'm offering to restructure the cli code to add the subcommands and alias (it requires bumping cobra and pflag to get spf13/cobra#295) if the majority is fine with the proposal.

[sstarcher]

@databus23 I'm 100% on board with your proposal I think it makes the most sense.

[foklepoint]

@databus23 Fine with your proposal, feel free to take over this PR if you need to. I don't have enough cycles to address the changes proposed here in the short term, but we'd definitely love to start using upstream helm-diff again :)

[databus23]

I pushed the proposed changes to master (helm diff chart became helm diff upgrade).
I also added the --version flag to diff upgrade.

Would anybody care to test this and give some feedback if they see any problem with it, especially in regards to backward compatibility?

[sstarcher]

@databus23 looks like helm diff is broken

Command "helm diff" is deprecated, use "helm diff upgrade" instead```

[sstarcher]

Tested out helm diff upgrade prometheus stable/prometheus --version 6.2.0 and that worked great.

[sstarcher]

also helm diff version looks good

[databus23]

@sstarcher That is expected. I added this encourage people to the the new subcommand but the command should still work. Is the command exiting cleanly? Then you maybe just have an empty diff.

[databus23]

So to clarify: helm diff should work exactly like helm upgrade diff only that it shows a deprecation notice. @sstarcher is that not working for you?

[sstarcher]

@databus23 yep, my mistake I assumed it existed with a error, but you are correct. It all LGTM.

[sstarcher]

@databus23 ahh the issue I saw was a different issue. Looks like helm diff does not work work helm s3

Command "helm diff" is deprecated, use "helm diff upgrade" instead
Error: Get s3://CHART/charts/CHART-0.1.1.tgz: unsupported protocol scheme "s3"
Usage:
  diff [flags]
  diff [command]

Available Commands:
  upgrade     Show a diff explaining what a helm upgrade would change.
  version     Show version of the helm diff plugin

Flags:
  -h, --help                   help for diff
      --no-color               remove colors from the output
      --reset-values           reset the values to the ones built into the chart and merge in any new values
      --reuse-values           reuse the last release's values and merge in any new values
      --set stringArray        set values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)
      --suppress stringArray   allows suppression of the values listed in the diff output
  -q, --suppress-secrets       suppress secrets in the output
  -f, --values valueFiles      specify values in a YAML file (can specify multiple) (default [])
      --version string         specify the exact chart version to use. If this is not specified, the latest version is used

Additional help topics:
  diff

Use "diff [command] --help" for more information about a command.

Error: plugin "diff" exited with error

I'll open a separate ticket

[sstarcher]

@foklepoint I believe this should now be closed due to the changes added to master

[databus23]

I believe this has been fixed by a536dfc and is shipped since version 2.9.0+1 of this plugin.