🐛 Fix issue found to re-generate the scaffolds with the new alpha command and adding e2e tests #3461

yyy1000 · 2023-06-19T11:53:14Z

Description:

Add e2e tests for kubebuilder alpha generate command.

Motivation:

Validate the functionality and the behavior of the new alpha command that has been merged.

k8s-ci-robot · 2023-06-19T11:53:23Z

Hi @yyy1000. 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/test-infra repository.

camilamacedo86 · 2023-06-20T04:16:24Z

/ok-to-test

camilamacedo86 · 2023-06-20T08:43:27Z

@yyy1000,

PLease squash the commits and rebase it with master so that we can move forward with this one

camilamacedo86 · 2023-06-20T08:49:14Z

test/e2e/alphagenerate/e2e_suite_test.go

+// Run e2e tests using the Ginkgo runner.
+func TestE2E(t *testing.T) {
+	RegisterFailHandler(Fail)
+	fmt.Fprintf(GinkgoWriter, "Starting kubebuilder alpha generate suite\n")


Suggested change

fmt.Fprintf(GinkgoWriter, "Starting kubebuilder alpha generate suite\n")

fmt.Fprintf(GinkgoWriter, "Starting kubebuilder suite test for the alpha command generate\n")

camilamacedo86 · 2023-06-20T08:50:49Z

test/e2e/alphagenerate/generate_test.go

+			kbc.Destroy()
+		})
+
+		It("should generate a runnable project with alpha generate", func() {


Suggested change

It("should generate a runnable project with alpha generate", func() {

It("should regenerate the project with success", func() {

We are not testing that the result is runnable so I think we need to change the test description

yyy1000 · 2023-06-20T09:25:57Z

I seems that the e2e tests in CI indicate other error not related to the change in this PR.

yyy1000 · 2023-06-20T09:27:24Z

/retest

camilamacedo86 · 2023-06-20T09:40:44Z

test/e2e/alphagenerate/generate_test.go

+	)
+	ExpectWithOffset(1, err).NotTo(HaveOccurred())
+
+	fileContainsExpr, err := pluginutil.HasFileContentWith(


Suggested change

fileContainsExpr, err := pluginutil.HasFileContentWith(

By("checking if the project file was generated with the expected content")

fileContainsExpr, err := pluginutil.HasFileContentWith(

Let's add a description so we have further info when fails to know where it failed

That's a good idea.

camilamacedo86 · 2023-06-20T09:41:32Z

test/e2e/alphagenerate/generate_test.go

+		- go.kubebuilder.io/v4
+		projectName: testdir
+		repo: my.domain/hello
+		version: "3"`)


I think it is easier to keep maintained if we create a const could we use a const instead?
See the other places for example.
It might be failing due missing spaces and etc.

camilamacedo86

I added so comments but you are doing great !!!

camilamacedo86 · 2023-06-20T09:41:58Z

test/e2e/alphagenerate/generate_test.go

+	)
+	ExpectWithOffset(1, err).NotTo(HaveOccurred())
+
+	fileContainsExpr, err := pluginutil.HasFileContentWith(


Let's add a description so we have further info when fails to know where it failed

camilamacedo86 · 2023-06-20T09:45:17Z

test/e2e/alphagenerate/generate_test.go

+		layout:
+		- go.kubebuilder.io/v4
+		projectName: testdir
+		repo: my.domain/hello


The repo, domain will not be this one since when we use the testContext and generate the project we create a domain and etc for the test. Therefore, see that

The domain will be kbc.Domain so you can use format string to pass the const with %s and replace the domain with the expected value

I would suggest you remove the repo: my.domain/hello

Sorry for this problem. I set domain to this because I tested in local, and whatever the old PROJECT file is. The domain of the new PROJECT file is always 'my.domain'. Maybe there's some problem with my implementation of 'alpha generate'.

camilamacedo86

I just change the title but it seems great for me 🥇
Good work !!

It has my approval,

Lets see if @varshaprasad96 and @Kavinjsir has none any extra considerations and if we have their lgtm as well.

Thank you

k8s-ci-robot · 2023-06-25T08:50:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, yyy1000

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:

~~OWNERS~~ [camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

varshaprasad96

Great work @yyy1000. Just a few nit questions.

varshaprasad96 · 2023-06-28T14:52:11Z

pkg/rescaffold/migrate.go

+		args = append(args, "--plugins")
+		args = append(args, plugins...)


could we do all the appends together?

Suggested change

args = append(args, "--plugins")

args = append(args, plugins...)

args = append(args, "--plugins", plugins...)

No, I tried this and it will not compile:

too many arguments in call to append have ([]string, string, []string) want ([]string, ...string) make: *** [Makefile:57: build] Error 1

varshaprasad96 · 2023-06-28T14:52:40Z

pkg/rescaffold/migrate.go

+		args = append(args, "--domain")
+		args = append(args, domain)


Similarly, does this work?

Suggested change

args = append(args, "--domain")

args = append(args, domain)

args = append(args, "--domain", domain)

This works! I think it's because when using append method, you either append multiple single element or a slice of elements. But can't mix them. :)

varshaprasad96 · 2023-06-28T14:54:19Z

test/e2e/alphagenerate/generate_test.go

+	//nolint:golint
+	//nolint:revive


Do we need to disable the linters here? Looks like we don't in other places and it works (https://github.com/kubernetes-sigs/kubebuilder/blob/47e6cab305336e1a7a5d577dc3c1500a445a70d1/pkg/cli/version_test.go#L20C1-L21)?

Ah, I just copy the code from other e2e tests code. I removed them, ran 'make lint' command and it was OK. I think I can also remove other unnecessary linters in another PR. :)

feat: add test data fix: add test in setup fix: change the file to PROJECT fix: test file update fix: doc nit and test data feat: Add By to check and project file feat: separate fields feat: use domain when scafflod feat: add plugin arg when re-scaffold fix: function name feat: refactor plugins fix: remove lint fix: domain append

@yyy1000

All seems most addressed so moving forward here for @yyy1000 does not get blocked.

camilamacedo86

/lgtm

camilamacedo86 · 2023-07-03T13:01:55Z

/pull-kubebuilder-e2e-k8s-1-27-1
/pull-kubebuilder-e2e-k8s-1-25-3

yyy1000 · 2023-07-04T14:26:54Z

/retest

camilamacedo86 · 2023-07-05T10:38:38Z

/test pull-kubebuilder-e2e-k8s-1-27-1
/test pull-kubebuilder-e2e-k8s-1-25-3

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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 Jun 19, 2023

k8s-ci-robot requested review from camilamacedo86 and varshaprasad96 June 19, 2023 11:53

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 20, 2023

camilamacedo86 reviewed Jun 20, 2023

View reviewed changes

yyy1000 force-pushed the e2e branch from ddbae81 to 19c3a9e Compare June 20, 2023 09:08

camilamacedo86 reviewed Jun 20, 2023

View reviewed changes

camilamacedo86 requested a review from Kavinjsir June 21, 2023 12:21

yyy1000 force-pushed the e2e branch 3 times, most recently from b45192b to c81f978 Compare June 25, 2023 05:08

camilamacedo86 changed the title ~~🌱 Add e2e tests for alpha generate command~~ 🐛 Fix issue found to re-generate the scaffolds with the new alpha command and adding e2e tests Jun 25, 2023

camilamacedo86 approved these changes Jun 25, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2023

varshaprasad96 previously requested changes Jun 28, 2023

View reviewed changes

yyy1000 force-pushed the e2e branch from c81f978 to 0ac8288 Compare June 29, 2023 01:42

yyy1000 force-pushed the e2e branch from 0ac8288 to 141f762 Compare June 29, 2023 01:43

camilamacedo86 reviewed Jul 3, 2023

View reviewed changes

k8s-ci-robot assigned camilamacedo86 Jul 3, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2023

k8s-ci-robot merged commit a632400 into kubernetes-sigs:master Jul 5, 2023
17 checks passed

yyy1000 deleted the e2e branch July 5, 2023 11:40

yyy1000 mentioned this pull request Aug 29, 2023

REQUEST: New membership for yyy1000 kubernetes/org#4417

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix issue found to re-generate the scaffolds with the new alpha command and adding e2e tests #3461

🐛 Fix issue found to re-generate the scaffolds with the new alpha command and adding e2e tests #3461

yyy1000 commented Jun 19, 2023

k8s-ci-robot commented Jun 19, 2023

camilamacedo86 commented Jun 20, 2023

camilamacedo86 commented Jun 20, 2023

camilamacedo86 Jun 20, 2023

camilamacedo86 Jun 20, 2023

yyy1000 commented Jun 20, 2023

yyy1000 commented Jun 20, 2023

camilamacedo86 Jun 20, 2023

camilamacedo86 Jun 20, 2023

yyy1000 Jun 20, 2023

camilamacedo86 Jun 20, 2023

camilamacedo86 left a comment

camilamacedo86 Jun 20, 2023

camilamacedo86 Jun 20, 2023

yyy1000 Jun 20, 2023

camilamacedo86 left a comment •

edited

Loading

k8s-ci-robot commented Jun 25, 2023

varshaprasad96 left a comment

varshaprasad96 Jun 28, 2023

yyy1000 Jun 29, 2023

varshaprasad96 Jun 28, 2023

yyy1000 Jun 29, 2023

varshaprasad96 Jun 28, 2023

yyy1000 Jun 29, 2023

camilamacedo86 left a comment

camilamacedo86 commented Jul 3, 2023

yyy1000 commented Jul 4, 2023

camilamacedo86 commented Jul 5, 2023

	fmt.Fprintf(GinkgoWriter, "Starting kubebuilder alpha generate suite\n")
	fmt.Fprintf(GinkgoWriter, "Starting kubebuilder suite test for the alpha command generate\n")

	It("should generate a runnable project with alpha generate", func() {
	It("should regenerate the project with success", func() {

	fileContainsExpr, err := pluginutil.HasFileContentWith(
	By("checking if the project file was generated with the expected content")
	fileContainsExpr, err := pluginutil.HasFileContentWith(

		args = append(args, "--plugins")
		args = append(args, plugins...)

	args = append(args, "--plugins")
	args = append(args, plugins...)
	args = append(args, "--plugins", plugins...)

	args = append(args, "--domain")
	args = append(args, domain)
	args = append(args, "--domain", domain)

🐛 Fix issue found to re-generate the scaffolds with the new alpha command and adding e2e tests #3461

🐛 Fix issue found to re-generate the scaffolds with the new alpha command and adding e2e tests #3461

Conversation

yyy1000 commented Jun 19, 2023

Description:

Motivation:

k8s-ci-robot commented Jun 19, 2023

camilamacedo86 commented Jun 20, 2023

camilamacedo86 commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyy1000 commented Jun 20, 2023

yyy1000 commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 25, 2023

varshaprasad96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 commented Jul 3, 2023

yyy1000 commented Jul 4, 2023

camilamacedo86 commented Jul 5, 2023

camilamacedo86 left a comment •

edited

Loading