-
Notifications
You must be signed in to change notification settings - Fork 1.6k
🐛 fix(helm): propagate project-name into init helm #4901
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
🐛 fix(helm): propagate project-name into init helm #4901
Conversation
Without a project-name, the generated helm chart was invalid. Signed-off-by: Mario Constanti <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bavarianbidi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @bavarianbidi. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
@@ -51,6 +51,8 @@ type Config interface { | |||
// SetProjectName sets the project name. | |||
// This method was introduced in project version 3. | |||
SetProjectName(name string) error | |||
// GenerateProjectName generates a project name. | |||
GenerateProjectName(name string) error |
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.
Thanks for the contribution!
We don't need this change because the project name is already tracked in the config, and there's no need to add a new method to the interface for it.
Just to clarify:
- The project name is already being set via
SetProjectName(name string) error
- And it's tracked in the config file. For example:
https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/PROJECT#L9
layout:
- go.kubebuilder.io/v4
projectName: project-v4 <- HERE WE HAVE THE PROJECT name
Hope that makes sense! 🙏
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.
yes, but if you do a kubebuilder init --plugins=helm/v1-alpha
in an empty directory there is no project name and a project-name isn't generated. And that's why the generated helm-charts are wrong.
My thought was not duplicating code we already have in kustomize init
(https://github.com/kubernetes-sigs/kubebuilder/pull/4901/files#diff-c81dda43edfad86b44d0309ad6026ae67301b5e2e6da69cf6f3cd59d3c7b89d4L72-L85) and do the same in helm init
as well.
but sure, i can revert that change and copy the same steps from kustomize init
to helm init
(beside the domain-stuff: https://github.com/kubernetes-sigs/kubebuilder/pull/4901/files#diff-c81dda43edfad86b44d0309ad6026ae67301b5e2e6da69cf6f3cd59d3c7b89d4L67-L69)
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.
just saw your comment #4901 (comment)
|
||
By("initializing a project") | ||
err = kbc.Init( | ||
"--plugins", "helm.kubebuilder.io/v1-alpha", |
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.
Thanks for the effort here!
Just a heads-up — this isn't quite accurate.
To properly test this, we need to mock a full project setup, which means running the following steps:
kubebuilder init
kubebuilder create api
kubebuilder create webhook
- Uncomment kustomize config if needed
- Then call the Helm plugin (e.g.,
kubebuilder edit --plugins helm
)
This sequence is necessary to simulate a real-world use case.
✅ Here's an example of the full project setup being mocked:
https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/v4/generate_test.go#L32-L68
📦 And here’s how we properly test Helm support after that setup:
https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/v4/plugin_cluster_test.go#L158-L181
Let me know if you'd like help aligning this test with the above pattern 🙌
err = kbc.Init( | ||
"--plugins", "helm.kubebuilder.io/v1-alpha", | ||
) | ||
Expect(err).NotTo(HaveOccurred(), "Failed to initialize project") |
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.
Thanks for raising this — I think I now understand your point about whether we should support helm
in the init
interface or only in edit
.
We definitely need to review this.
Technically, you can run kubebuilder init --plugins=go/v4,helm/v1-alpha1
, which includes Helm during project initialization — but the Helm-specific content wouldn't be fully or properly populated at that stage.
That’s because Helm relies on the Go project being scaffolded first.
Originally, the intention was to allow us to automatically update Helm-related files using edit
, since the default layout is always called in the plugin chain.
For example, if we init a project with plugins A, B, and C — when we later call create api
, all those plugins will be executed in the chain automatically.
So with that in mind, it seems clearer now that having a helm init
doesn't really make sense.
I think the right path is to remove the init
implementation for Helm and keep only edit
. Thanks again for bringing this up — I now understand the confusion you raised earlier on Slack. Would you mind opening an issue for this? And would you be interested in working on it? 🙂
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.
I think the right path is to remove the init implementation for Helm and keep only edit. Thanks again for bringing this up — I now understand the confusion you raised earlier on Slack. Would you mind opening an issue for this? And would you be interested in working on it? 🙂
Thanks for that. That was exactly what i wanted to know 😅
I'm definitely open to implement this.
Will raise a new issue and with that we can close my previous issue #4899 afterwards
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.
removal-request for helm init
created #4902
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.
Thank you a lot 🎉
That is amazing !!!
I added some comments please feel free to reach out if you need any further clarification.
Without a project-name, the generated helm chart was invalid.
Therefor it's required to have the
project-name
as flag in theinit
subcommand.If
project-name
is not defined, the project-name will get generated by the name of the current working directory.This is already done in the
kustomization init
. Therefore I've extended the config-Interface byGenerateProjectName
and implemented a generic func which is then used inkustomization init
as well as inhelm init
.Added tests for the util-package as I wanted to have a faster feedback loop during implementation and e2e tests for
--plugins=helm/v1-alpha
case.closes #4899