Skip to content
This repository has been archived by the owner on Feb 15, 2022. It is now read-only.

Helm 3 Support #296

Merged
merged 13 commits into from
Apr 29, 2020
Merged

Helm 3 Support #296

merged 13 commits into from
Apr 29, 2020

Conversation

michaelperel
Copy link
Contributor

closes #291

Summary

  • This PR shells out to the Helm 3 binary rather than consuming the go library because the go library's commands are based on unexported logic that we would have to duplicate in this project.
  • This PR drops support for Helm 2, in place of Helm 3. Is this desired behavior? If not, it is possible to maintain both.
  • I made this PR by looking through Changes since Helm 2 docs. Relevant to this project, I noticed:
    • Helm 3 removes Tiller
    • Helm 3 gets rid of $HELM_HOME
    • Helm 3 supports dependencies in Chart.yaml and requirements.yaml
    • Helm 3 does not include the stable repository by default
  • I tested the changes by running the tests. For me, TestHelmMethodInstall fails sometimes on master, and it fails in the same way on this branch. I do not have access to the build pipeline. Since this project is open source, would it be possible to open the build pipeline to everyone?
  • I have updated the comment about the installing helm chart workaround for Helm 2. I confirmed that helm template works as expect in Helm 3, but I did not want to update the design (potentially not install helm charts) because that seemed like a big design decision.
  • Values in cmd/generate_test.go were created by replacing heritage: Tiller with heritage: Helm as well as a few differences between Helm 2 and Helm 3 such as | vs |-

A quick exploration of why I think we should consume the Helm 3 binary

  • Only necessary to read if you care why I chose to shell out instead of consume the library

I have triaged the helm 3 client repo a bit and think it makes much more sense to actually shell out to the helm 3 binary rather than use the client library directly because the commands such as "helm template" have a lot of unexported logic that we would need to reimplement if we wanted to be consistent with the "helm template" behavior.

For instance, here is the "template" command:

Among other things that occur for the template command to run with appropriate warnings, this unexported runInstall function runs, and we would need to copy its logic before actually using the exported function (whose logic is here).

Additionally, I thought of taking the approach of just using their logic that builds the cobra root function. Unfortunately, that is unexported as well.

@timfpark timfpark requested a review from evanlouie April 23, 2020 15:22
@evanlouie
Copy link
Contributor

evanlouie commented Apr 23, 2020

Hey @michaelperel, thanks for the PR!
We might need to hold off on merging this until we can give a clear roadmap to cover any breaking changes from helm2 to helm3 (don't want to break any pipelines that are using this).

This PR drops support for Helm 2, in place of Helm 3. Is this desired behavior? If not, it is possible to maintain both.

This might be the best solution to move this along. Would it be a big ask to add this? I'm thinking we would want to support both versions until a 1.0 release

@michaelperel
Copy link
Contributor Author

michaelperel commented Apr 23, 2020

Hey @evanlouie, you are welcome!

If you review this PR as it is and think the approach is fine, I will look into adding back Helm 2 (and put it in this PR). I think it should be fairly straightforward.

On another note, I bumped the golangci-lint version to the most recent version because I was running into this issue. Bumping the version shows no linting errors on my side (though I suspect still in the pipeline because even though I cannot see the pipeline, it is failing quickly).

@michaelperel
Copy link
Contributor Author

Thanks @timfpark for access to the pipeline, much appreciated!

@evanlouie In the previous 2 commits, I fixed the linter by adding a timeout and updated the random directory logic so they no longer collide. Since all tests pass now, I think the PR is ready for review.

I have thought a bit more about adding back helm 2 support. Since I cannot imagine a case where somebody would use Fabrikate with helm 2 and helm 3 at the same time, I was wondering, could there perhaps be two versions either using semver+go mod or branches where all new features go into the helm 3 version, and bug fixes are ported back to helm 2?

IMO, this would protect logic from duplicating/special casing (saying if helm 2 do X, else do Y), and would start a clear path towards deprecating helm 2.

@evanlouie
Copy link
Contributor

evanlouie commented Apr 24, 2020

I'm not against reving a major for helm3 support. We would need to refactor some bedrock scripts (e.g https://github.com/microsoft/bedrock/blob/master/gitops/azure-devops/build.sh) a bit to pin a major version (currently just pulls latest binary). But I do agree that it would be cleaner to push towards a full migration towards helm 3 and drop helm 2.

@andrebriggs thoughts?

cmd/install.go Show resolved Hide resolved
@evanlouie evanlouie changed the base branch from master to develop April 29, 2020 20:49
Copy link
Contributor

@evanlouie evanlouie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! thanks for the contribution :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consume helm 3 go lib instead of shelling out to helm
2 participants