Skip to content

Refactor cmd/helm to allow library usage#13617

Merged
joejulian merged 5 commits into
helm:mainfrom
AustinAbro321:refactor-cmd
Feb 27, 2025
Merged

Refactor cmd/helm to allow library usage#13617
joejulian merged 5 commits into
helm:mainfrom
AustinAbro321:refactor-cmd

Conversation

@AustinAbro321

@AustinAbro321 AustinAbro321 commented Jan 9, 2025

Copy link
Copy Markdown
Contributor

This is reopening #12725

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.

I do not believe this to be a breaking change as anything that was in cmd/helm was 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 NewRootCmd this included the debug/warning log functions and the PluginError. Would love a discussion and I'm happy to modify/resubmit as needed.

If applicable:

  • this PR contains user facing changes (the docs needed label should be applied if so)
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility - refactor, no behavior behavior changes

@TerryHowe

Copy link
Copy Markdown
Contributor

Are you looking for a user experience like https://oras.land/docs/1.3.0-beta.1/how_to_guides/go_script

The associated PR oras-project/oras#825

@AustinAbro321

Copy link
Copy Markdown
Contributor Author

@TerryHowe Somewhat. The implementation is essentially the same, expose the root command. However, I plan to use the root command so I can embed it into a tool I maintain called Zarf, which simplifies air-gap Kubernetes. We do a similar embedding of Kubectl, see - https://github.com/zarf-dev/zarf/blob/fc183abb20353c8d4ce3ba2ecc16c6190e5dbecf/src/cmd/tools/kubectl.go#L23, which allows us to make kubectl callable using the zarf binary with zarf tools kubectl

@AustinAbro321 AustinAbro321 marked this pull request as ready for review January 27, 2025 15:50
@AustinAbro321 AustinAbro321 changed the title export cmd as a library export NewRootCmd Jan 27, 2025
@AustinAbro321 AustinAbro321 changed the title export NewRootCmd Refactor cmd/helm to allow library usage #12725 Jan 27, 2025
@AustinAbro321 AustinAbro321 changed the title Refactor cmd/helm to allow library usage #12725 Refactor cmd/helm to allow library usage Jan 27, 2025
Comment thread pkg/cmd/root.go Outdated
Comment thread pkg/cmd/root.go Outdated

@TerryHowe TerryHowe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/lgtm

Comment thread SECURITY.md

@joejulian joejulian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just need to restore the SECURITY.md. The rest looks good to me.

joejulian
joejulian previously approved these changes Feb 21, 2025

@joejulian joejulian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

@joejulian joejulian added this to the v4 milestone Feb 21, 2025
@joejulian

Copy link
Copy Markdown
Contributor

@AustinAbro321 looks like it needs a rebase already.

@AustinAbro321

AustinAbro321 commented Feb 22, 2025

Copy link
Copy Markdown
Contributor Author

@joejulian Rebased, but looks like it's failing from a flake, I'm able to repeat the flake on main #30573

@AustinAbro321

Copy link
Copy Markdown
Contributor Author

@joejulian @TerryHowe Looks like the flake was fixed, should be good now

@joejulian

Copy link
Copy Markdown
Contributor

Can you squash this, please. That tar file will remain in the data that would get merged.

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321

Copy link
Copy Markdown
Contributor Author

Done

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

@gjenkins8 gjenkins8 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(no changes to PR since last approval)

@joejulian joejulian merged commit 726b2f6 into helm:main Feb 27, 2025
@TerryHowe

Copy link
Copy Markdown
Contributor

A quick example in the docs would bring awareness to this feature.

@Hammond95

Copy link
Copy Markdown

@AustinAbro321 Has this been backported to v3 as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs needed Indicates that a PR includes changes that need to be documented. Should not block a PR being merged dont-backport feature v4.x Issues and Pull Requests related to the major version v4

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants