Skip to content
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

H4HIP: Move pkgs to be internal #384

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gjenkins8
Copy link
Member

No description provided.

@gjenkins8 gjenkins8 changed the title H4HIP: Move implementation packages to be internal/private H4HIP: Move pkgs to be internal Feb 2, 2025
Copy link
Contributor

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

The action packages provide the business logic the Helm CLI uses in a reusable manner. But, other pieces of software aren't required to use them and often don't. The other packages are regularly used by software using the SDK.

For example...

  • The Flux helm-controller directly uses many different packages on the proposed list to move.
  • The helm-mapkubeapis plugin, maintained by the Helm project, uses packages outside of action. It needs to get at the releases and make changes to them which requires using the building block packages.

I'm aware of numerous other applications that use a wide variety of Helm packages.

We can't anticipate all their needs to create higher level abstractions (e.g., in the action package) and it would be too much work to act on all their requests. This is why providing access to the building blocks is useful. I don't think we should make them internal.

@gjenkins8
Copy link
Member Author

gjenkins8 commented Feb 9, 2025

So yes, the goal is to (more) fully try and determine what can be moved/made internal

  1. What can be moved outright e.g. pkg/gates. Do we really expect SDKs users to consume this package?
  2. What can be reduced, e.g. pkg/engine has a few callers to Render(). Similar to chartutil for chartutil.Values and chartutil.ProcessDependenciesWithMerge. We don't need to expose significant internal for a few APIs
  3. Similarly, Helm's public SDK should focus on APIs design on public consumption. With the original lift-and-shift (as I understand it) of Helm's internal workings to create an SDK, I suspect a lot of these packages were made public without thorough review of the suitability of the APIs to be public facing.

An alternative or prelude to this HIP might be to document what Helm should expect to be public too...


Taking a look at flux2 (or practically helm-controller). I see a variety of the above scenarios:

https://github.com/search?q=repo%3Afluxcd%2Fhelm-controller+%22helm.sh%2Fhelm%2Fv3%22&type=code&p=1

// Packages which meet the criteria to be public (well defined API, suitable purpose)
"helm.sh/helm/v3/pkg/action"
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/chart/loader"
"helm.sh/helm/v3/pkg/release"
"helm.sh/helm/v3/pkg/storage"
"helm.sh/helm/v3/pkg/storage/driver"
"helm.sh/helm/v3/pkg/time" // can (almost) be removed now: https://tip.golang.org/doc/go1.24 / `omitzero`

// Packages IMHO that should not be exposed (Ill defined API)
"helm.sh/helm/v3/pkg/chartutil"
"helm.sh/helm/v3/pkg/releaseutil"

// Packages IMHO that should not be exposed (Internal purpose, limited external utility)
"helm.sh/helm/v3/pkg/kube"

Flux2 (helm-controller) needs:

  • chartutil for chartutil.Values type and chartutil.ProcessDependenciesWithMerge only
  • releaseutil for releaseutil.SortByRevision() only
  • kube for kube.ResourceList type; test code that uses kube.Client (doesn't use the client however); and setting the field manager name (kube.ManagedFieldsManager = controllerName).
  • pkg/postrender for postrender.PostRenderer interface type only (but proceeds to create its own PostRenderer implementations)

So while e.g. flux2 does need Helm SDK APIs beyond actions. A significant potion of exposed APIs I still maintain are not suitable to external consumption, or a simply not used.

Further analysis can be done of course. Initial goal here is to get an agreement as to how to proceed with classifying APIs to consider moving them

@mattfarina
Copy link
Contributor

I want to add some context to a few packages...

  • helm.sh/helm/v3/pkg/kube was added to simplify things1. Because of this, numerous other applications use this package. Many, but not all, of those cases are alongside Helm's other packages. In order to make this internal we would need to change the API so this is never needed to be exposed. Today it is. Making it internal right now would stop applications from doing things they do today. Why do we want to remove that? How will we ensure all the API changes so they aren't blocked with the Helm v4 SDK doing the actions they already do?
  • helm.sh/helm/v3/pkg/chartutil and helm.sh/helm/v3/pkg/releaseutil are utility packages alongside the chart and release packages. These were created when Helm v2 was doing protobuf and it had Tiller on the server side. For example, chartutil worked with github.com/kubernetes/helm/pkg/proto/hapi/chart for charts. There was also the pkg/charts package. I wonder if we can move the work of chartutil and releaseutil into the respective chart and release packages. Thoughts?

Footnotes

  1. It simplified things in 2 ways. First, when k8s packages have changes it can lead to updates over a lot of different points in the code. Updating one place while using the adapter pattern reduced this. The second way is that some common things were turned into functions.

@mattfarina
Copy link
Contributor

Note, the gates package I was initially thinking would be good for internal. But, there are calls to expose the CLI itself as a package. Will this mean people need/want the gates?

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

Successfully merging this pull request may close these issues.

2 participants