-
Notifications
You must be signed in to change notification settings - Fork 1.6k
✨ Add helm/v2-alpha addressing all feedbacks and aiming maintainability. Deprecated helm/v1-alpha in favor of helm/v2-alpha #5058
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
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cb1ee39
to
1acfef2
Compare
d9d172d
to
2a5c5be
Compare
8203eed
to
1cb0ec1
Compare
1cb0ec1
to
2040599
Compare
16b3fca
to
1863d2e
Compare
ca52223
to
5610db1
Compare
fs.BoolVar(&p.force, "force", false, "if true, regenerates all the files") | ||
fs.StringVar(&p.manifestsFile, "manifests", DefaultManifestsFile, | ||
"path to the YAML file containing Kubernetes manifests from kustomize output") | ||
fs.StringVar(&p.outputDir, "output", DefaultOutputDir, "output directory for the generated Helm chart") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the flag called output-dir
?
as you suggested in my initial PR here: #4891 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
returned from PTO this week, will do a deeper review, starting next week, if wanted. |
688b258
to
070f1d9
Compare
@camilamacedo86 many thanks for the PR. I've tested this version and it already looks very promising to my prior issues. I just found some things to improve:
This a special directory introduced especially for crds in helm. By doing this, helm will install these crds if not exists by default and you can skip this with
I think using
Besides these small findings I think the new plugin version helm/v2-alpha is a huge improvement! Thank you |
070f1d9
to
1841bc8
Compare
Hi @mkarlheim The chart. labels were added 👍 to allow adding extra labels. Regards the CRDs, see: Although Helm best practices recommend placing CRDs under a top-level This was a conscious design decision aafter lengthydiscussions in the design proposal and community threads (Slack discussion link). The reason: CRDs in Alternatively, here we could create a chart only for the CRDs, such as a two-chart flow (API first, then operator/app), with a simple readiness/wait check. However, if we change it, it would be better in a follow-up discussion where we can address it. Let's keep the cards under the templates, which is the most adopted approach. |
73643da
to
bcfad58
Compare
…`) that dynamically generates Helm charts based on the actual kustomize output from `make build-installer`, replacing the previous hardcoded template approach in `helm/v1-alpha`. The existing `helm/v1-alpha` plugin used static templates that didn't reflect user customizations (environment variables, labels, annotations, security contexts, etc.) made in their kustomize configuration. This led to inconsistencies between `kubectl apply -f dist/install.yaml` and `helm install`. - Deprecated Helm v1-alpha in favour of v2 - Add docs and tests for Helm v2 - Update all samples - Address all feedbacks raised so far Assisted-by: OpenAI
bcfad58
to
d41081b
Compare
@@ -34,6 +34,7 @@ import ( | |||
autoupdatev1alpha1 "sigs.k8s.io/kubebuilder/v4/pkg/plugins/optional/autoupdate/v1alpha" | |||
grafanav1alpha1 "sigs.k8s.io/kubebuilder/v4/pkg/plugins/optional/grafana/v1alpha" | |||
helmv1alpha1 "sigs.k8s.io/kubebuilder/v4/pkg/plugins/optional/helm/v1alpha" | |||
helmv2alpha1 "sigs.k8s.io/kubebuilder/v4/pkg/plugins/optional/helm/v2alpha" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about enforcing this import-pattern via golangci-lint by the importas section (https://golangci-lint.run/docs/linters/configuration/#importas).
With that we get a consistent import section ...
I'm fine if we do this in a dedicated PR (have it on my list and happy to raise a PR after that)
@@ -34,6 +34,7 @@ import ( | |||
autoupdate "sigs.k8s.io/kubebuilder/v4/pkg/plugins/optional/autoupdate/v1alpha" | |||
"sigs.k8s.io/kubebuilder/v4/pkg/plugins/optional/grafana/v1alpha" | |||
hemlv1alpha "sigs.k8s.io/kubebuilder/v4/pkg/plugins/optional/helm/v1alpha" | |||
hemlv2alpha "sigs.k8s.io/kubebuilder/v4/pkg/plugins/optional/helm/v2alpha" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hemlv2alpha "sigs.k8s.io/kubebuilder/v4/pkg/plugins/optional/helm/v2alpha" | |
helmv2alpha "sigs.k8s.io/kubebuilder/v4/pkg/plugins/optional/helm/v2alpha" |
- the line above
enforcing import-names might help here as well (see #5058 (comment))
Thanks for clarifying. |
Introduce helm/v2-alpha Plugin: Dynamic Helm Chart Generation
This PR introduces a completely rewritten Helm plugin (
helm/v2-alpha
) that dynamically generates Helm charts based on the actual kustomize output frommake build-installer
, replacing the previous hardcoded template approach inhelm/v1-alpha
.The existing
helm/v1-alpha
plugin used static templates that didn't reflect user customizations (environment variables, labels, annotations, security contexts, etc.) made in their kustomize configuration. This led to inconsistencies betweenkubectl apply -f dist/install.yaml
andhelm install
.Key Features
dist/install.yaml
)metrics/
,webhook/
,cert-manager/
,rbac/
,crd/
)Changes
Documentation
📖 Preview docs: https://deploy-preview-5058--kubebuilder.netlify.app/plugins/available/helm-v2-alpha
How to Review
pkg/plugins/optional/helm/v2alpha/
for the actual code changestestdata/
anddocs/
are sample updates with helm/v2-alphaUsage
Closes: