Closed michaelperel closed 4 years ago
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
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).
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.
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?
closes #291
Summary
cmd/generate_test.go
were created by replacingheritage: Tiller
withheritage: 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
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.