Refactor cmd/helm to allow library usage#12725
Closed
Conversation
DO NOT SUBMIT Signed-off-by: Eddie Zaneski <eddiezane@gmail.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DO NOT SUBMIT - need to fix a few tests still
What this PR does / why we need it:
The Zarf project currently ships additional vendored tools that may be needed in an airgapped environment. Helm doesn't expose the root command and makes use of internal APIs (version for example) so we are a bit stuck at the moment with attempting to vendor a quasi-fork in tree.
It looks like someone else was attempting to do something similar #12122.
This PR refactors the code base to allow for importing the root Helm command as library code. I modeled it after how we structured kubectl.
https://github.com/kubernetes/kubernetes/blob/f40ff3d9ffb2a06ebb256b6e4c5d1b62079bb3b9/cmd/kubectl/kubectl.go#L29
https://github.com/kubernetes/kubernetes/blob/f40ff3d9ffb2a06ebb256b6e4c5d1b62079bb3b9/staging/src/k8s.io/kubectl/pkg/cmd/cmd.go#L100
I do not believe this to be a breaking change as anything that was in
cmd/helmwas part of a main package and not exported. I think the bigger question is what surface (if any) do you want to expose.Here's an example of what this looks like to be used.
Special notes for your reviewer:
My initial goal here is to find out if y'all are open to a change like this - doesn't have to be in this shape. My approach was to publicly expose the bare minimum to make it possible to embed the root Helm command. Aside from the
NewRootCmdthis included the debug/warning log functions and thePluginError. Would love a discussion and I'm happy to modify/resubmit as needed.If applicable:
cc @marckhouzam @arikmaor