⚠️ Update ginkgo@v2.1.1 and gomega@v1.18.0 #1792

acumino · 2022-02-05T10:20:16Z

Migrate ginkgo to v2.

Important Note:
-> Per node timeout has been removed to prevent the wrongly marking a test as failure which is caused by some other test. Read more about it here
-> Custom reporters have been deprecated in ginkgo v2 and will be removed in future releases.

k8s-ci-robot · 2022-02-05T10:20:24Z

Welcome @acumino!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. 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/controller-runtime 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-05T10:20:25Z

Hi @acumino. 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-05T10:20:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: acumino
To complete the pull request process, please assign alvaroaleman after the PR has been reviewed.
You can assign the PR to them by writing /assign @alvaroaleman 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

acumino · 2022-02-05T15:02:15Z

/invite @FillZpp

acumino · 2022-02-08T13:18:23Z

/ping @alvaroaleman

FillZpp · 2022-02-11T06:08:18Z

/ok-to-test

acumino · 2022-02-11T09:08:21Z

/test pull-controller-runtime-test-master
/test pull-controller-runtime-apidiff-master

alvaroaleman

It seems this doesn't build?

hack/test-all.sh

pkg/builder/builder_suite_test.go

alvaroaleman · 2022-02-16T18:27:09Z

/retest

The error you are seeing in CI is apparently golangci-lints way of saying "Your code doesn't" build, i.E. retesting is not going to help. Ref: golangci/golangci-lint#825

acumino · 2022-03-03T10:09:43Z

/retest all

schrej

From what I can see, the removal of those two helpers for gingko v1 in pkg/envtest/printer makes this a breaking change.

Yes, Ginkgo does a major bump, but it is possible to have both v1 and v2 as a dependency in the same project. Bumping Ginkgo alone only influences the tests of controller-runtime, but not the tests of dependant projects. Cluster API is still using Ginkgo v1 and works fine when running its test with this PR.
Since Cluster API doesn't use the two removed helpers, it's not affected by this PR at all.

The removal of those helpers are also the only changes happening in files that do not end with _test.go. Any change inside of _test.go files will not have an effect on projects using controller-runtime as a dependency.

cc @camilamacedo86 (thanks for staying persistent! I totally missed those helpers on the first review.)

schrej · 2022-05-23T13:49:06Z

pkg/envtest/printer/prow.go

-// see https://github.com/onsi/ginkgo/issues/706
-// When using this you must make sure to grep for failures in your junit xmls and fail the run
-// if there are any.
-func NewProwReporter(suiteName string) ginkgo.Reporter {


Removing this makes it a breaking change.
https://cs.github.com/?scopeName=All+repos&scope=&q=+printer.NewProwReporter

schrej · 2022-05-23T13:56:27Z

pkg/envtest/printer/ginkgo.go

-// NewlineReporter is Reporter that Prints a newline after the default Reporter output so that the results
-// are correctly parsed by test automation.
-// See issue https://github.com/jstemmer/go-junit-report/issues/31
-type NewlineReporter struct{}


Removing this makes it a breaking change.
https://cs.github.com/?scopeName=All+repos&scope=&q=+printer.NewLineReporter

See that you are changing the test_.go files. Those with controller runtime projects will also need to change their _test.go files as well, right? They might use methods that will be affected or not. For example, the default scaffolds done by Kubebuilder will be affected if we need to bump v2.

So, the correct mojo here would be ⚠️.

They will need to change their tests if they are using either printer.NewLineReporter() or printer.NewProwReporter().

Any change that we do in our _test.go files does not affect any users of controller-runtime. Code in _test.go files can't be imported, so it also can't affect their implementation. That's why I initially thought this is not a breaking change.

I would propose to move the removal of these two reporters to a separate PR, so this one does not have to be breaking. And then maybe postpone the removal of those helpers a bit to allow more time for migration to v2.

This is only a temporary adaption, as we are using the temporary compatibility layer, which will be removed again soon. Hence, we have to rework this and adapt for the long-term.

k8s-ci-robot · 2022-06-03T14:16:19Z

@acumino: 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-controller-runtime-test-master	`1a677d0`	link	true	`/test pull-controller-runtime-test-master`

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.

k8s-ci-robot · 2022-06-18T00:35:25Z

@acumino: PR needs rebase.

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.

matteoolivi · 2022-07-28T22:29:39Z

Why was this closed? Can someone provide an explanation/a status update?

alvaroaleman · 2022-07-29T00:54:50Z

@matteoolivi presumably bceause @acumino didn't have the cycles to finish it

matteoolivi · 2022-08-10T07:39:39Z

@alvaroaleman . Oh. I can definitely relate to that.

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 5, 2022

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 5, 2022

k8s-ci-robot requested review from alvaroaleman and vincepri February 5, 2022 10:20

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 Feb 11, 2022

alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 16, 2022

alvaroaleman reviewed Feb 16, 2022

View reviewed changes

hack/test-all.sh Outdated Show resolved Hide resolved

pkg/builder/builder_suite_test.go Outdated Show resolved Hide resolved

pkg/builder/builder_suite_test.go Show resolved Hide resolved

acumino force-pushed the ginkgo-v2 branch 5 times, most recently from 5c3fcbb to bba0864 Compare February 16, 2022 20:00

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2022

acumino force-pushed the ginkgo-v2 branch from bba0864 to beb43d9 Compare March 3, 2022 08:32

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2022

acumino force-pushed the ginkgo-v2 branch from 7e41873 to e42ec8b Compare March 3, 2022 10:01

acumino requested a review from alvaroaleman March 3, 2022 15:08

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2022

acumino force-pushed the ginkgo-v2 branch from e42ec8b to 489a8a1 Compare March 14, 2022 18:07

schrej reviewed May 23, 2022

View reviewed changes

acumino changed the title ~~✨ Update ginkgo@v2.1.1 and gomega@v1.18.0~~ ⚠️ Update ginkgo@v2.1.1 and gomega@v1.18.0 May 24, 2022

acumino added 13 commits May 24, 2022 13:55

Upgrade to ginkgo v2.1.1

92b5ae5

Adapt ginkgo import paths

ed8df81

Drop obsolete table extension imports

6bf50a9

tidy

fed90d2

Remove {Before,After} Suite specific timeout

d491a45

Remove It specific timeout

486e94d

Adapt custom reporter to changed reporting infrastructure

c784cdf

This is only a temporary adaption, as we are using the temporary compatibility layer, which will be removed again soon. Hence, we have to rework this and adapt for the long-term.

Update dependency

3ba9150

Address PR Review

f225518

Adpat custom reporter chnages

46ea91f

Minor edit

50edea9

Rebase

a266794

Rebase

1a677d0

acumino force-pushed the ginkgo-v2 branch from 489a8a1 to 1a677d0 Compare June 3, 2022 14:10

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2022

sbueringer mentioned this pull request Jul 13, 2022

⚠️ Update tests to Ginkgo v2 kubernetes-sigs/cluster-api#6906

Merged

acumino closed this Jul 18, 2022

schrej mentioned this pull request Aug 11, 2022

⚠ Migrate to ginkgo v2 #1977

Merged

acumino deleted the ginkgo-v2 branch February 1, 2023 18:29

This was referenced Feb 7, 2023

Update dependencies kubernetes-retired/kubefed#1534

Closed

Update dependencies kubesphere/kubefed#1

Merged

Bump sigs.k8s.io/controller-runtime to v0.14.4 kubesphere/kubesphere#5507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Update ginkgo@v2.1.1 and gomega@v1.18.0 #1792

⚠️ Update ginkgo@v2.1.1 and gomega@v1.18.0 #1792

acumino commented Feb 5, 2022 •

edited

Loading

k8s-ci-robot commented Feb 5, 2022

k8s-ci-robot commented Feb 5, 2022

k8s-ci-robot commented Feb 5, 2022

acumino commented Feb 5, 2022

acumino commented Feb 8, 2022

FillZpp commented Feb 11, 2022

acumino commented Feb 11, 2022

alvaroaleman left a comment

alvaroaleman commented Feb 16, 2022

acumino commented Mar 3, 2022 •

edited

Loading

schrej left a comment •

edited

Loading

schrej May 23, 2022 •

edited

Loading

schrej May 23, 2022 •

edited

Loading

camilamacedo86 May 24, 2022

schrej May 24, 2022

k8s-ci-robot commented Jun 3, 2022

k8s-ci-robot commented Jun 18, 2022

matteoolivi commented Jul 28, 2022

alvaroaleman commented Jul 29, 2022

matteoolivi commented Aug 10, 2022

⚠️ Update ginkgo@v2.1.1 and gomega@v1.18.0 #1792

⚠️ Update ginkgo@v2.1.1 and gomega@v1.18.0 #1792

Conversation

acumino commented Feb 5, 2022 • edited Loading

k8s-ci-robot commented Feb 5, 2022

k8s-ci-robot commented Feb 5, 2022

k8s-ci-robot commented Feb 5, 2022

acumino commented Feb 5, 2022

acumino commented Feb 8, 2022

FillZpp commented Feb 11, 2022

acumino commented Feb 11, 2022

alvaroaleman left a comment

Choose a reason for hiding this comment

alvaroaleman commented Feb 16, 2022

acumino commented Mar 3, 2022 • edited Loading

schrej left a comment • edited Loading

Choose a reason for hiding this comment

schrej May 23, 2022 • edited Loading

Choose a reason for hiding this comment

schrej May 23, 2022 • edited Loading

Choose a reason for hiding this comment

camilamacedo86 May 24, 2022

Choose a reason for hiding this comment

schrej May 24, 2022

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 3, 2022

k8s-ci-robot commented Jun 18, 2022

matteoolivi commented Jul 28, 2022

alvaroaleman commented Jul 29, 2022

matteoolivi commented Aug 10, 2022

acumino commented Feb 5, 2022 •

edited

Loading

acumino commented Mar 3, 2022 •

edited

Loading

schrej left a comment •

edited

Loading

schrej May 23, 2022 •

edited

Loading

schrej May 23, 2022 •

edited

Loading