-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/* | ||
Copyright 2025 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package helm | ||
|
||
import ( | ||
"fmt" | ||
"testing" | ||
|
||
. "github.com/onsi/ginkgo/v2" | ||
. "github.com/onsi/gomega" | ||
) | ||
|
||
// Run e2e tests using the Ginkgo runner. | ||
func TestE2E(t *testing.T) { | ||
RegisterFailHandler(Fail) | ||
_, _ = fmt.Fprintf(GinkgoWriter, "Starting helm plugin kubebuilder suite\n") | ||
RunSpecs(t, "Kubebuilder helm plugin e2e suite") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
/* | ||
Copyright 2025 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package helm | ||
|
||
import ( | ||
"path/filepath" | ||
|
||
pluginutil "sigs.k8s.io/kubebuilder/v4/pkg/plugin/util" | ||
|
||
. "github.com/onsi/ginkgo/v2" | ||
|
||
. "github.com/onsi/gomega" | ||
|
||
"sigs.k8s.io/kubebuilder/v4/test/e2e/utils" | ||
) | ||
|
||
var _ = Describe("kubebuilder", func() { | ||
Context("plugin helm/v1-alpha", func() { | ||
var kbc *utils.TestContext | ||
|
||
BeforeEach(func() { | ||
var err error | ||
kbc, err = utils.NewTestContext(pluginutil.KubebuilderBinName, "GO111MODULE=on") | ||
Expect(err).NotTo(HaveOccurred()) | ||
Expect(kbc.Prepare()).To(Succeed()) | ||
}) | ||
|
||
AfterEach(func() { | ||
kbc.Destroy() | ||
}) | ||
|
||
It("should generate a runnable project with helm plugin", func() { | ||
GenerateProject(kbc) | ||
}) | ||
}) | ||
}) | ||
|
||
// GenerateProject implements a helm/v1(-alpha) plugin project defined by a TestContext. | ||
func GenerateProject(kbc *utils.TestContext) { | ||
var err error | ||
|
||
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 commentThe 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.
This sequence is necessary to simulate a real-world use case. ✅ Here's an example of the full project setup being mocked: 📦 And here’s how we properly test Helm support after that setup: Let me know if you'd like help aligning this test with the above pattern 🙌 |
||
) | ||
Expect(err).NotTo(HaveOccurred(), "Failed to initialize project") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 We definitely need to review this. Technically, you can run 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 For example, if we init a project with plugins A, B, and C — when we later call So with that in mind, it seems clearer now that having a I think the right path is to remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thanks for that. That was exactly what i wanted to know 😅 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 commentThe reason will be displayed to describe this comment to others. Learn more. removal-request for |
||
|
||
fileContainsExpr, err := pluginutil.HasFileContentWith( | ||
filepath.Join(kbc.Dir, "PROJECT"), | ||
`helm.kubebuilder.io/v1-alpha: {}`) | ||
Expect(err).NotTo(HaveOccurred(), "Failed to edit sum rate for custom metrics") | ||
Expect(fileContainsExpr).To(BeTrue(), "Failed to find helm plugin in PROJECT file") | ||
|
||
fileContainsExpr, err = pluginutil.HasFileContentWith( | ||
filepath.Join(kbc.Dir, "PROJECT"), | ||
"projectName: e2e-"+kbc.TestSuffix) | ||
Expect(err).NotTo(HaveOccurred(), "Failed to edit sum rate for custom metrics") | ||
Expect(fileContainsExpr).To(BeTrue(), "Failed to find projectName in PROJECT file") | ||
|
||
fileContainsExpr, err = pluginutil.HasFileContentWith( | ||
filepath.Join(kbc.Dir, "dist", "chart", "Chart.yaml"), | ||
"name: e2e-"+kbc.TestSuffix) | ||
Expect(err).NotTo(HaveOccurred(), "Failed to edit sum rate for custom metrics") | ||
Expect(fileContainsExpr).To(BeTrue(), "Failed to find name in Chart.yaml file") | ||
|
||
fileContainsExpr, err = pluginutil.HasFileContentWith( | ||
filepath.Join(kbc.Dir, "dist", "chart", "templates", "manager", "manager.yaml"), | ||
`metadata: | ||
name: e2e-`+kbc.TestSuffix) | ||
Expect(err).NotTo(HaveOccurred(), "Failed to edit sum rate for custom metrics") | ||
Expect(fileContainsExpr).To(BeTrue(), "Failed to find name in helm template manager.yaml file") | ||
|
||
} |
Uh oh!
There was an error while loading. Please reload this page.
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:
SetProjectName(name string) error
https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/PROJECT#L9
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 inhelm init
as well.but sure, i can revert that change and copy the same steps from
kustomize init
tohelm 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)