✨ Upgrade ginkgo to v2 #2522

haoxins · 2022-02-28T08:34:47Z

No description provided.

k8s-ci-robot · 2022-02-28T08:34:55Z

Welcome @haoxins!

It looks like this is your first PR to kubernetes-sigs/kubebuilder 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubebuilder has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-02-28T08:34:55Z

Hi @haoxins. 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.

k8s-ci-robot · 2022-02-28T08:35:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: haoxins
To complete the pull request process, please assign pwittrock after the PR has been reviewed.
You can assign the PR to them by writing /assign @pwittrock in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

camilamacedo86 · 2022-03-02T11:23:40Z

/ok-to-test

pkg/plugins/golang/v2/scaffolds/internal/templates/controllers/controller_suitetest.go

camilamacedo86 · 2022-03-02T12:44:28Z

docs/book/src/reference/envtest.md

@@ -104,9 +104,7 @@ var _ = BeforeSuite(func(done Done) {

 	_, err := testenv.Start()
 	Expect(err).NotTo(HaveOccurred())
-
-	close(done)
-}, 60)


Why the done was removed here?

https://onsi.github.io/ginkgo/MIGRATING_TO_V2#removed-async-testing

Shows that the migration for this one is:

It("...", func() { done := make(chan interface{}) go func() { // user test code to run asynchronously close(done) //signifies the code is done }() Eventually(done, timeout).Should(BeClosed()) })

yeah, if async is important for us, we can do that.
Also, custom reports~

camilamacedo86 · 2022-03-02T12:45:11Z

testdata/project-v3-config/controllers/suite_test.go

 	. "github.com/onsi/gomega"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
-	"sigs.k8s.io/controller-runtime/pkg/envtest/printer"


why the printer was removed?

camilamacedo86 · 2022-03-02T12:45:36Z

testdata/project-v3-config/controllers/suite_test.go

@@ -44,9 +43,7 @@ var testEnv *envtest.Environment
 func TestAPIs(t *testing.T) {
 	RegisterFailHandler(Fail)

-	RunSpecsWithDefaultAndCustomReporters(t,
-		"Controller Suite",
-		[]Reporter{printer.NewlineReporter{}})


Why we are changing the default scaffold more than just the import?

https://onsi.github.io/ginkgo/MIGRATING_TO_V2#migration-strategy-2

So, how could we achieve the same with the v2? Are we able to provide it via the scaffolds? How will it impact the author's projects?

Also, it shows a breaking change for authors' projects so the emoji needs to be changed to warning :

It looks like the migration for this involves adding a ReportAfterSuite node: https://onsi.github.io/ginkgo/MIGRATING_TO_V2#generating-custom-reports-when-a-test-suite-completes

Since we need to. change the scaffold then it shows a breaking change.

camilamacedo86 · 2022-03-02T12:51:36Z

testdata/project-v3-v1beta1/go.mod

-	github.com/onsi/ginkgo v1.16.4
-	github.com/onsi/gomega v1.13.0
+	github.com/onsi/ginkgo/v2 v2.1.3
+	github.com/onsi/gomega v1.17.0


This version is probably incompatible. We need to revert the dep if/when the v1beta1 be used see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/v3/commons.go#L38

Some nit changes, mainly

Ensure that we are updating the dep but does not change the default scaffold implementation

Ensure that the update is valid only for the go/v3 plugin and we will do the downgrade if/when v1beta1 CRD be a scaffold for backwards compatibilities.

Also, see:

The title/commit is used to create the release notes. then, this change affects the scaffold so we need to use the emoji :sparkles:. More info: https://github.com/kubernetes-sigs/kubebuilder/blob/master/CONTRIBUTING.md#pr-process

WDYT about : (go/v3) - Upgrade dep from github.com/onsi/ginkgo to github.com/onsi/ginkgo/v2

Maybe I can split the scaffold change into another PR?

@camilamacedo86 I might be totally missing something, but what does the ginkgo version have to do with the CRD version? I would expect ginkgo/v2 to work with projects that use v1beta1 CRDs. Is it just that the other dependencies we're required to use with v1beta1 CRDs use ginkgo/v1, so we're concerned that there would be issues if the kubebuilder project used v2, but the project's dependencies used v1?

The one important thing to cover is this:

Regardless of the CRD version (or anything else), if a project was originally scaffolded with ginkgo v1, we should continue scaffolding new APIs and webhooks with ginkgo v1.

I'm not sure I saw anything for that in my review, but if we have that covered, disregard.

When we scaffold CRD v1beta1 we need to ensure that we use compatible deps with controller-gen v0.6.2 and controller-runtime v0.9.2 since these are the latest versions where we can use v1beta1 with. So, my point is that:

We need to check if the ginkgo v2 is compatible with or not, if not we need to try to downgrade (here)

However, we checked in the review that changes in the scaffolds are required and that the previous code will not work with ginkgov2 so it is a breaking change and then we cannot move forward in go/v3. We will need a go/v4 plugin to add this one. I am thinking that we should track it to address with go/v4, see: https://github.com/kubernetes-sigs/kubebuilder/milestone/26

@joelanford wdyt?

/hold

(it shows a breaking change and cannot be addressed on go/v3)

camilamacedo86

Some nit changes, mainly

Ensure that we are updating the dep but does not change the default scaffold implementation
Ensure that the update is valid only for the go/v3 plugin and we will do the downgrade if/when v1beta1 CRD be a scaffold for backwards compatibilities.

Also, see:

The title/commit is used to create the release notes. then, this change affects the scaffold so we need to use the emoji :sparkles:. More info: https://github.com/kubernetes-sigs/kubebuilder/blob/master/CONTRIBUTING.md#pr-process
WDYT about : (go/v3) - Upgrade dep from github.com/onsi/ginkgo to github.com/onsi/ginkgo/v2

haoxins · 2022-03-02T13:27:14Z

@camilamacedo86 I just reverted the change to scaffold so that I can finish the plugin/v3 change tomorrow.
I prefer to do that in another PR, but fine if you think they should be done as a whole.

haoxins · 2022-03-03T06:40:22Z

Some nit changes, mainly

Ensure that we are updating the dep but does not change the default scaffold implementation

Ensure that the update is valid only for the go/v3 plugin and we will do the downgrade if/when v1beta1 CRD be a scaffold for backwards compatibilities.

Also, see:

The title/commit is used to create the release notes. then, this change affects the scaffold so we need to use the emoji :sparkles:. More info: https://github.com/kubernetes-sigs/kubebuilder/blob/master/CONTRIBUTING.md#pr-process

WDYT about : (go/v3) - Upgrade dep from github.com/onsi/ginkgo to github.com/onsi/ginkgo/v2

Updated~

revert useless change revert chore change v3 scaffolds base on CRD version fix fix fix apply fix

k8s-ci-robot · 2022-03-03T07:27:18Z

@haoxins: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubebuilder-e2e-k8s-1-15-12	`59d4527`	link	true	`/test pull-kubebuilder-e2e-k8s-1-15-12`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

camilamacedo86

Hi @haoxins,

We checked in the review that changes in the scaffolds are required and that the previous code will not work with ginkgo v2 so it is a breaking change and then we cannot move forward in go/v3. We will need a go/v4 plugin to add this one. I am thinking that we should track it to address with go/v4, see: https://github.com/kubernetes-sigs/kubebuilder/milestone/26. WDYT Could you help us out by doing that and linking this PR?

See:

Also, you can check the policy: https://github.com/kubernetes-sigs/kubebuilder/blob/master/VERSIONING.md#introducing-changes-to-plugins

@joelanford wdyt? Have you objections to it?

haoxins · 2022-03-09T03:42:26Z

👍 If async and custom reports are required for us, I can implement these changes.
I just created an issue for this PR to track the upgrade for go/v4 milestone and other discussions, see Upgrade ginkgo to v2 for go/v4 plugin #2532.

camilamacedo86 · 2022-03-09T13:29:33Z

Hi @haoxins,

I am closing this one since we can plan that for go/v4 and we could check that is a breaking change and I hope that you do not mind. Thank you it will be helpful for we achieve it with go/v4 as well.

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 28, 2022

k8s-ci-robot requested review from camilamacedo86 and pwittrock February 28, 2022 08:35

haoxins changed the title ~~mod: Upgrade ginkgo to v2~~ 🌱 Upgrade ginkgo to v2 Feb 28, 2022

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 Mar 2, 2022

camilamacedo86 reviewed Mar 2, 2022

View reviewed changes

pkg/plugins/golang/v2/scaffolds/internal/templates/controllers/controller_suitetest.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Mar 2, 2022

View reviewed changes

camilamacedo86 requested changes Mar 2, 2022

View reviewed changes

haoxins changed the title ~~🌱 Upgrade ginkgo to v2~~ ✨ Upgrade ginkgo to v2 Mar 2, 2022

haoxins requested a review from camilamacedo86 March 3, 2022 06:40

mod: Upgrade ginkgo to v2

59d4527

revert useless change revert chore change v3 scaffolds base on CRD version fix fix fix apply fix

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2022

camilamacedo86 requested changes Mar 8, 2022

View reviewed changes

haoxins mentioned this pull request Mar 9, 2022

Upgrade ginkgo to v2 for go/v4 plugin #2532

Closed

camilamacedo86 closed this Mar 9, 2022

haoxins deleted the ginkgo-v2 branch March 9, 2022 16:34

jakobmoellerdev mentioned this pull request Aug 16, 2022

🌱 migrate kubebuilder CLI tests to ginkgo/v2 (this pr does not change the scaffolds done by the tool) #2876

Merged

camilamacedo86 mentioned this pull request Aug 16, 2022

Migrate to ginkgo v2 #2875

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Upgrade ginkgo to v2 #2522

✨ Upgrade ginkgo to v2 #2522

haoxins commented Feb 28, 2022

k8s-ci-robot commented Feb 28, 2022

k8s-ci-robot commented Feb 28, 2022

k8s-ci-robot commented Feb 28, 2022

camilamacedo86 commented Mar 2, 2022

camilamacedo86 Mar 2, 2022

haoxins Mar 2, 2022

camilamacedo86 Mar 8, 2022

haoxins Mar 9, 2022

camilamacedo86 Mar 2, 2022

camilamacedo86 Mar 2, 2022

haoxins Mar 2, 2022

camilamacedo86 Mar 2, 2022

joelanford Mar 3, 2022

camilamacedo86 Mar 8, 2022 •

edited

Loading

camilamacedo86 Mar 2, 2022

haoxins Mar 2, 2022

joelanford Mar 3, 2022

camilamacedo86 Mar 8, 2022 •

edited

Loading

camilamacedo86 left a comment

haoxins commented Mar 2, 2022

haoxins commented Mar 3, 2022

k8s-ci-robot commented Mar 3, 2022 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

haoxins commented Mar 9, 2022

camilamacedo86 commented Mar 9, 2022

✨ Upgrade ginkgo to v2 #2522

✨ Upgrade ginkgo to v2 #2522

Conversation

haoxins commented Feb 28, 2022

k8s-ci-robot commented Feb 28, 2022

k8s-ci-robot commented Feb 28, 2022

k8s-ci-robot commented Feb 28, 2022

camilamacedo86 commented Mar 2, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Mar 8, 2022 • edited Loading

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 Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

haoxins commented Mar 2, 2022

haoxins commented Mar 3, 2022

k8s-ci-robot commented Mar 3, 2022 • edited Loading

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

haoxins commented Mar 9, 2022

camilamacedo86 commented Mar 9, 2022

camilamacedo86 Mar 8, 2022 •

edited

Loading

camilamacedo86 Mar 8, 2022 •

edited

Loading

k8s-ci-robot commented Mar 3, 2022 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading