Skip to content

feat: Experimental support for Helm 3.0.0-beta.3 #149

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

Closed
wants to merge 1 commit into from

Conversation

mumoshu
Copy link
Collaborator

@mumoshu mumoshu commented Sep 7, 2019

Tested with helm diff upgrade only. Changes for other helm-diff sub-commands will be made in coming weeks.

Changes:

  • Migrated from Glide to Go modules as helm-diff was unable to build with Glide due to bunch of missing transitive dependencies probably due to that some projects are not managed by Glide
  • Auto-detection of the plugin context(helm 2 or 3)
  • helm diff upgrade has been enhanced so that it turns into Helm 3 mode

Pre-requisites:

  • You need to helm plugin install with helm of 3.0.0-x as the plugin installation directory has been changed from that of helm 2.x.

Tested with `helm diff upgrade` only. Changes for other helm-diff sub-commands will be made in coming weeks.

Changes:

- Migrated from Glide to Go modules as helm-diff was unable to build with Glide due to bunch of missing transitive dependencies probably due to that some projects are not managed by Glide
- Auto-detection of the plugin context(helm 2 or 3)
- `helm diff upgrade` has been enhanced so that it turns into Helm 3 mode

Pre-requisites:

- You need to `helm plugin install` with `helm` of 3.0.0-x as the plugin installation directory has been changed from that of helm 2.x.

Ref #147
@databus23
Copy link
Owner

databus23 commented Sep 7, 2019

@mumoshu ❤️ I started looking into this yesterday. Will review/test first thing on Monday.

@mumoshu
Copy link
Collaborator Author

mumoshu commented Sep 8, 2019

@databus23 Hey! Thanks for your response ☺️

I'd appreciate it if you could also take a look into #150 as well. The implementation is similar except that how much changes are made to existing sources, and #150 has a broader feature coverage!

@mumoshu
Copy link
Collaborator Author

mumoshu commented Sep 8, 2019

@databus23 Btw incorporating helm 3 dependencies triples helm-diff binary size:

helm 2 ver:

$ ls -lah ~/.helm/plugins/helm-diff/bin/diff
-rwxr-xr-x  1 c-ykuoka  staff    16M  9  6 23:04 /Users/c-ykuoka/.helm/plugins/helm-diff/bin/diff

helm 3 ver(build from this PR):

$ ls -lah ~/Library/helm/plugins/helm-diff/bin/diff
-rwxr-xr-x  1 c-ykuoka  staff    50M  9  7 17:19 /Users/c-ykuoka/Library/helm/plugins/helm-diff/bin/diff

I have no idea how we can avoid this without improving things on helm-side.

Initially I thought this should be like around 30M and removing the helm2 support in the future bring it back to the original size. But I now realize that it can be impossible. We' need to import almost everything from helm3 as helm3 does more on client-side, compared to that the dependency was basically API and tiller client only for helm2.

Anyways, I just wanted to let you know.

@databus23
Copy link
Owner

databus23 commented Sep 9, 2019

@mumoshu Thanks again for starting to work on this.

We' need to import almost everything from helm3 as helm3 does more on client-side

This is an important point I've been thinking about in the past days. I think this poses a deeper problem than the size of the binary. Now with tiller gone, and all the rendering done locally the plugin would need to be compiled against the same version of the helm binary being used. Otherwise there might be differences in how templates are rendered. For helm diff this means the diff could be different from what is actually applied by the helm binary.
While tiller was a pain in general it was very beneficial for this plugin that the rendering happened remote on the server with the same engine that rendered the chart when upgrading a chart with the main binary (tiller).
Even if we would get our act together and publish this plugin in lockstep with helm versions there could still be weird problems when a user has multiple versions of helm3 installed for multiple clusters because they all share the same plugin folder.

I personally would like to avoid the situation where we need to keep up with helm's release cycle. I quite like that this plugin uses an older version of helm and still works with newer versions of helm without the need to rebuild.

Currently I see two options that could avoid this for helm3:

I see pros and cons in both approaches. Keeping this a separate plugin allows us to move quicker and add more features and subcommands then if this would be part of the official helm codebase. On the other hand having diff be a builtin feature like helm template would be very nice from a users perspective.
What do you think?

@jonasrutishauser
Copy link
Contributor

Currently I see two options that could avoid this for helm3:

  • We shell out to helm template to generate the manifest
  • We try to integrate diff as a proper subcommand of the main binary as suggested here:(helm/helm#3156 (comment)

The first option would be simple to implement such that the same code would work for both helm versions.
In this case we could use helm template and/or helm get to get the manifests.
This way it should be mostly independent of helm's release cycle and the binary can stay small.

There is only one thing which bothers me. Helm 3 uses now 3-way strategic merge patches and i don't know how this plugin should handle this.

Nevertheless if i have some time i will try to implement a PR for the first option.

@mumoshu
Copy link
Collaborator Author

mumoshu commented Sep 10, 2019

@databus23 Good point!

I like the first option as the starting point as it seems more feasible and allows users to migrate to helm 3 earlier.

In the longer term upstreaming diff definitely make sense. However it would take time as it likely require us to write some proposal and talk about maintenance(I believe we should stand as maintainers of the upstreamed diff sub-command but not sure how the process goes. So trying to address helm 3 support with upstreaming requires us to resolve two problems together, implementation and governance things.

@jonasrutishauser
Copy link
Contributor

If created #151 for the first option.

@databus23
Copy link
Owner

Closed in favour of #151

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.

3 participants