support helm 3 using exec

[jonasrutishauser]

Here is an other version for #147 which shells out to HELM_BIN.

The following helm commands are used:

• helm template for update
• helm get to retrieve releases and revisions

The following flags are not supported in helm 3 mode:

• --set-file in upgrade
• --no-hooks in upgrade (helm template in v3.0.0-beta.3 ignores this flag and returns always no hooks)
• --include-tests in all commands as we ignore all hooks

It would be possible to get the hooks for the installed releases/revisions if it is really needed.

[mumoshu]

--set-file in upgrade

Yeah I'm thinking that this has been moved to something like --set file:///path/to/the/file(not tested

--no-hooks in upgrade (helm template in v3.0.0-beta.3 ignores this flag and returns always no hooks)

I guess this is a bug in helm3 as helmv3 template --help stills shows it as available(and not deprecated):

      --no-hooks                 prevent hooks from running during install

[mumoshu]

@jonasrutishauser Thanks a lot for your contribution! This looks good to me.

@databus23 I think this is awesome at least as the starter/foundation to further improvements. Would you mind taking a look, too?

[mumoshu]

Just submitted the fix for helm template --no-hooks in helm 3 here: helm/helm#6444

[thomastaylor312]

So --set-file has now been ported over and will be available in beta.4.

As for the --no-hooks flag, I am not seeing that --no-hooks was ever available in Helm 2 for helm template. I think the --no-hooks in helm 3 is coming from the generic way we add flags (I think we add all of the install flags onto the template command). Is the desire here to have helm template output hooks if so desired?

EDIT: So we already output hooks as part of the default output in Helm 2, so the desired behavior is as @mumoshu suggests, correct? Just have a way to exclude hooks if so desired

[mumoshu]

So --set-file has now been ported over and will be available in beta.4.

@thomastaylor312 Thanks a lot for your effort ☺️

So we already output hooks as part of the default output in Helm 2, so the desired behavior is as @mumoshu suggests, correct? Just have a way to exclude hooks if so desired

Yep you are correct! And that's exactly what I did in helm/helm#6444

[databus23]

Hi sorry for being super late to the party! I'll start reviewing this now.
Right of the start I noticed that somehow --kube-context and --namespace do not work anymore. With helm2 this was transparently handled by the main binary as it was responsible for making the connection to Kubernetes.
I tried:
helm3 --kube-context diff ... --> unknown flag: --kube-context
and
helm3 diff ... --kube-context --> unknown flag: --kube-context

As we shell out to HELM_BIN we need to figure out a way to pass both ---kube-context and --namespace to the underlying helm calls.

Sorry again for delay.

[thomastaylor312]

@databus23 I think we may actually be binding the same config flags to Helm soon, which should fix that issue. I'll get back to you when I figure it out

[thomastaylor312]

This PR is the one: helm/helm#6508

Also, the namespace flag should definitely work for helm 3. Is this beta 3 that you are running?

[databus23]

@thomastaylor312 --namespace does not produce the unknown flag error. But It seems the plugin does not get handed the value handed down via environment. The namespace flag already defined in this plugin https://github.com/databus23/helm-diff/pull/151/files#diff-39563e4504a4a921d3883674e71cdeafR100 is also not working anymore (its always is set the default default), presumably because the helm binary is snatching the flag before invoking the plugin.
So it seems to me currently a helm3 plugin is unaware of --kube-context and --namespace passed to the invocation.

@jonasrutishauser Have you tested your PR against a different namespace then default? I can't get it to work (using helm version beta.3)

[thomastaylor312]

Yeah, we aren't passing down namespace or context from helm into the plugin. If this is something that needs to be passed down, let's open an issue in Helm and I'll tag it properly

[mumoshu]

Ah good finding. I think anything related to tiller(kubeconfig, kubecontext, namespace=tiller-namespace) except connection options(port, tls, etc.) should be propagated to plugins via envvars. In helm 2 plugins are given those configs implicitly via TILLER_HOST and the local proxy to tiller started by helm for the plugin.

And I believe the only viable option is passing via envvars, as other options like helm calling a plugin with --namespace doesn't work if the plugin does not implement the flag(hence not interested in the namespace).

[databus23]

I believe HELM_NAMESPACE will be coming with the PR helm/helm#6508: https://github.com/helm/helm/pull/6508/files#diff-f3e6a920d4f94bb29be3f3a1eb084f24R104.

This leaves passing --kube-context to plugins.

[databus23]

it seems the flag processing for the plugins was changed so that --context instead of --kube-context is removed from the list of arguments:
helm/helm@19398a2#diff-98149b6c69be4e8ba0603746b9437188R111

I don't really understand how this is supposed to work. the main binary still exposes --kube-context as the canonical flag to select which context to use of the kubeconfig. I would expect that invoking plugins would work using the same flag.

[jonasrutishauser]

I updated the PR to support HELM_NAMESPACE, --set-file and --no-hooks flag.

It needs the following PRs merged in helm:

• ref(*): refactor flag and environment settings helm/helm#6508
• Use same output format for hooks as for manifest helm/helm#6582

[mumoshu]

Reading helm/helm#6508 (comment), I noticed that --kubeconfig set in helm is also unavailable to plugins.

[databus23]

@mumoshu yeah, we should open a proper issue upstream to pass kubecontext and kubeconfig (probaply as KUBECONFIG) to plug-ins. Would be good to get this in before RCs drop. I‘m on vacation this week I’ll do so next week if nobody else has done it by then.

[mumoshu]

@databus23 Thanks for confirming!

I just opened helm/helm#6631 for that. Does it look ok to you?

[mumoshu]

@jonasrutishauser @databus23 HELM_NAMESPACE, KUBECONFIG, HELM_KUBECONTEXT has been added via helm/helm#6632

Perhaps the remaining things to do would be adding --kube-context $HELM_KUBECONTEXT if $HELM_KUBECONTEXT is not empty and --kubeconfig $KUBECONFIG if it's not empty, to the following functions?

• getRelease
• getHooks
• getRevision
• getChart
• template
• existingValues

[jonasrutishauser]

I think they must not be added, as they should be picked by helm from the environment.

There is a bigger problem as not all arguments are consistently removed by helm3.
I opened a PR helm/helm#6657 for that which should also add support for HELM_KUBECONTEXT.

The only environment variable we need to read is HELM_NAMESPACE to get the namespace.

[mumoshu]

I think they must not be added, as they should be picked by helm from the environment.

@jonasrutishauser Let me confirm - Without HELM_KUBECONTEXT read by helm-diff, how will you make helm-diff to know about e.g. the context when it is run like helm --kube-context myctx diff?

[jonasrutishauser]

helm-diff needs only to know the namespace as this is used in the code.

The other parameters are only needed to communicate with the cluster.
All this communication is done by helm.
As long as helm reads the same environment variables as it sets in plugin invocations it should work.

[mumoshu]

As long as helm reads the same environment variables as it sets in plugin invocations it should work.

@jonasrutishauser Ah, so in other words, you're willing to pass-through HELM_KUBECONEXT and HELM_KUBECONFIG set by helm that called helm-diff, to helm that is called by helm-diff, right?

Then, do you think helm/helm#6657 needs to be merged before helm v3 becomes GA?

The current situation seems like the PR is considered just a nice-to-have because it has no 3.0.0-beta.5 milestone set by the helm maintainers.

[thomastaylor312]

@mumoshu I am still working on looking through everything and understanding the behavior change. If we need to pull it in we will!

[jonasrutishauser]

I think this PR should now work with the next 3.0.0-beta.5 release.

The only flag in helm 3 mode no longer provided is --include-tests. I will try to add this functionality too.
If I'm correct the test hook (helm.sh/hook annotation) in helm 3 has the value test. Additional the value test-success is supported for backward compatibility.

[jonasrutishauser]

--include-tests flag is now supported too.

[mumoshu]

@thomastaylor312 @jonasrutishauser Thanks a lot for your continued efforts!

As it seems all green now, I'll take some time for manual testing on my end, and will proceed merging this shortly.

[databus23]

works in my short local tests. Thanks so much for this @jonasrutishauser and @mumoshu!
helm 3.0.0-beta.5 is imminent. As soon as its released I will release a 3.0.0-rc.1 version of the plugin.