Skip to content

🌱 helm(e2e): test helm plugin integration #4914

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

Merged

Conversation

bavarianbidi
Copy link
Contributor

this will test the usage and impact to an existing project by the kubebuilder edit command for the helm plugin.

This is a follow up from #4903

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 9, 2025
@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 9, 2025
@bavarianbidi bavarianbidi force-pushed the add_helm_edit_e2e_tests branch from 45fe90f to df9368a Compare July 9, 2025 06:36
)
Expect(err).NotTo(HaveOccurred(), "Failed to edit the project")

ensureCommonHelmFilesContent(kbc)
Copy link
Member

Choose a reason for hiding this comment

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

See: https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/alphagenerate/generate_test.go#L234-L257

We should load the Project file and check the data as above
We do not need to create any new function in the utils plugin or test for that.

We will validate after parsing the PROJECT file to the config ( marshall ) if we have the helm plugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should load the Project file and check the data as above

Will use some of the code where we marshal the config file.

For all the helm-template files I am checking for: I've decided to use the existing pluginutil.HasFileContentWith function. Didn't wanted to introduce a new dependency here but we could try to marshal the helm-files - it definitely feels better than doing this file-comparision-foo 😅

We do not need to create any new function in the utils plugin or test for that.

I think this might be also a dedicated issue.
What I currently see: there are a bunch of funcs which are quite generic (or at least need some small adjustments) to make them re-usable in the end2end-tests.

)

BeforeAll(func() {
err := os.MkdirAll("testdata", 0o755)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using test data here?
We should not touch there in the unit tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not the testdata folder from the repo-root.
It's a temporary testdata folder which get's generated during tests.

I've just re-used this folder-name as we already create that folder in the existing tests as well:
https://github.com/kubernetes-sigs/kubebuilder/pull/4914/files/df9368ad4c692d8af48f866066895eacf2de7232#diff-f8d5c1dc31041937679a6530c7e9779770025f88333231013b03bf0018cbb98cR35

// it is needed to run the `kubebuilder edit --plugins helm.kubebuilder.io/v1-alpha` command
// with ` --force` again to ensure that the webhooks are enabled in the
// values.yaml file.
It("should extend a runnable project with helm plugin and webhooks", func() {
Copy link
Member

Choose a reason for hiding this comment

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

Are we validating that the project is runnable?
No right, so could we try to improve the text to be more in line with what we are checking out?

Copy link
Contributor Author

@bavarianbidi bavarianbidi Jul 9, 2025

Choose a reason for hiding this comment

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

got your finding.
I've used the term runnable as we use this term in the kubebuilder-book as well as in the existing e2e-tests few times, e.g.:

[...] Note that most of this tutorial is generated from literate Go files that
form a runnable project, and live in the book source directory

But to be true, we mostly run the generated projects we are generating in the e2e-tests beside one project in the grafana-plugin. Unfortunately I've used the grafana-plugin e2e test as a basic for my e2e-tests 🙈

will re-phrase the description and the comments

Copy link
Member

Choose a reason for hiding this comment

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

We might need to improve it there as well -)

@camilamacedo86
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 11, 2025
@bavarianbidi bavarianbidi force-pushed the add_helm_edit_e2e_tests branch from df9368a to 73931df Compare July 14, 2025 15:06
this will test the usage and impact to an existing project by the kubebuilder edit command for the helm
plugin.

Signed-off-by: Mario Constanti <[email protected]>
@bavarianbidi bavarianbidi force-pushed the add_helm_edit_e2e_tests branch from 73931df to 98c60e7 Compare July 15, 2025 04:31
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2025
@camilamacedo86
Copy link
Member

Hi @bavarianbidi

I think we will need to improve those a little.
But we can either do that in follow ups
So, it might be great enough just to get started.

Thank you a lot for your contribution 🥇

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bavarianbidi, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2025
@k8s-ci-robot k8s-ci-robot merged commit 5ed6496 into kubernetes-sigs:master Jul 15, 2025
7 checks passed
@bavarianbidi
Copy link
Contributor Author

Hi @bavarianbidi

I think we will need to improve those a little.
But we can either do that in follow ups
So, it might be great enough just to get started.

Thank you a lot for your contribution 🥇

/lgtm
/approve

hi @camilamacedo86 , yes in the e2e code base is definitely room for improvement.

I'm fine with taking that with me.
Will create an issue first to document my findings and you can extend with your ideas. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants