Skip to content
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

✨ Update ginkgo (2.2.0 -> 2.4.0) and gomega (1.20.1 -> 1.22.1) #7490

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

oscr
Copy link
Contributor

@oscr oscr commented Nov 3, 2022

What this PR does / why we need it:

Updates gingko to 2.4.0, which also means bumping gomega to 1.22.1

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 3, 2022
@oscr oscr force-pushed the update-ginkgo-2.4.0 branch from b902b98 to e1e22e8 Compare November 3, 2022 21:45
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 3, 2022
@oscr
Copy link
Contributor Author

oscr commented Nov 3, 2022

/test ?

@k8s-ci-robot
Copy link
Contributor

@oscr: The following commands are available to trigger required jobs:

  • /test pull-cluster-api-build-main
  • /test pull-cluster-api-e2e-main
  • /test pull-cluster-api-test-main
  • /test pull-cluster-api-test-mink8s-main
  • /test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-apidiff-main
  • /test pull-cluster-api-e2e-full-main
  • /test pull-cluster-api-e2e-informing-ipv6-main
  • /test pull-cluster-api-e2e-informing-main
  • /test pull-cluster-api-e2e-workload-upgrade-1-25-latest-main

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-apidiff-main
  • pull-cluster-api-build-main
  • pull-cluster-api-e2e-informing-ipv6-main
  • pull-cluster-api-e2e-informing-main
  • pull-cluster-api-e2e-main
  • pull-cluster-api-test-main
  • pull-cluster-api-test-mink8s-main
  • pull-cluster-api-verify-main

In response to this:

/test ?

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.

@oscr
Copy link
Contributor Author

oscr commented Nov 3, 2022

/test pull-cluster-api-e2e-full-main

@oscr
Copy link
Contributor Author

oscr commented Nov 3, 2022

Failed due to: INFO: [WARNING] Unable to load image "quay.io/jetstack/cert-manager-cainjector:v1.10.0" into the kind cluster "clusterctl-upgrade-icvq74": failed to detect containerd snapshotter: command "docker exec --privileged clusterctl-upgrade-icvq74-control-plane-lsn4x containerd config dump" failed with error: exit status 126 Trying again.

/test pull-cluster-api-e2e-full-main

@@ -47,7 +47,7 @@ The default value is 0, meaning that the volume can be detached without any time

- The Kubernetes default registry has been changed from `k8s.gcr.io` to `registry.k8s.io`. Kubernetes image promotion currently publishes to both registries. Please
consider publishing manifests which reference the controller images from the new registry (for reference [Cluster API PR](https://github.com/kubernetes-sigs/cluster-api/pull/7478)).
- e2e tests are upgraded to use Ginkgo v2 (v2.2.0) and Gomega v1.20.1. Providers who use the test framework from this release will also need to upgrade, because Ginkgo v2 can't be imported alongside v1. Please see the [Ginkgo upgrade guide](https://onsi.github.io/ginkgo/MIGRATING_TO_V2), and note:
- e2e tests are upgraded to use Ginkgo v2 (v2.4.0) and Gomega v1.22.1. Providers who use the test framework from this release will also need to upgrade, because Ginkgo v2 can't be imported alongside v1. Please see the [Ginkgo upgrade guide](https://onsi.github.io/ginkgo/MIGRATING_TO_V2), and note:
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon Nov 3, 2022

Choose a reason for hiding this comment

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

Just to make sure, does this require any changes from providers? If so, it should go through our approval process (defined in https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/developer/release-cycle.md#release-cycle)

All changes that impact providers' adoption of the new release should be announced in the provider updates section of the office hours meeting notes and approved in the PR or issue by both approvers and key affected providers

No objections from my end, just thought I'd point it out since the migration guide is being updated and we the beta release has already been published.

Copy link
Member

@sbueringer sbueringer Nov 4, 2022

Choose a reason for hiding this comment

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

Good point and a good opportunity to refine further what "impact provider's adoption" means.

In my opinion changing dependencies ~ minor/pach version should be fine (I wouldn't bump k8s.io/, CR minor versions as they are not semver minor versions) even without approval as long as providers don't have to make code changes for that. If they would have to it should go through the approval process.

This means we would be fine with the fact that after providers are bumping to another beta and they are running go mod tidy some dependencies in their go.mod file change as well.

I looked through the release notes and they look good and non-disruptive to me.
That we didn't have to change anything in the test-framework or our e2e tests is another data point.
(gomega 1.23 would have made changes necessary > #7460 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for raising this important question Cécile. It's a great point. I also fully agree with Stefans excellent comment.

Copy link
Contributor

@ykakarap ykakarap left a comment

Choose a reason for hiding this comment

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

/lgtm

Tests are all green.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022
@sbueringer
Copy link
Member

lgtm pending resolution of #7490 (comment)

@sbueringer
Copy link
Member

Thx @oscr for opening this PR!

I think we should make sure we have relatively up-to-date dependencies in our minor releases.

Might be worth checking some of our other dependencies

@oscr
Copy link
Contributor Author

oscr commented Nov 4, 2022

@sbueringer Sounds great! Would you like it in this or another pr?

@sbueringer
Copy link
Member

sbueringer commented Nov 4, 2022

@sbueringer Sounds great! Would you like it in this or another pr?

Separate PR would be good. Thank you!

@oscr
Copy link
Contributor Author

oscr commented Nov 4, 2022

Separate PR would be good. Thank you!

No problem. I will get it done as soon as this merges to avoid having multiple prs changing the same files.

@sbueringer
Copy link
Member

sbueringer commented Nov 4, 2022

Separate PR would be good. Thank you!

No problem. I will get it done as soon as this merges to avoid having multiple prs changing the same files.

If you have time, can you already take a look what would change and maybe open a WIP-PR? (you could build it on top of this PR for simplicity)

Because depending on how much impact it has we have to discuss it in office hours again and in case we hold back this PR until Wednesday we would probably not have enough time to discuss other bumps before the RC on 15th November

Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 Nov 4, 2022
@k8s-ci-robot k8s-ci-robot merged commit 5b986d8 into kubernetes-sigs:main Nov 4, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.3 milestone Nov 4, 2022
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. 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.

6 participants