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.

Sign up for GitHub

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

Jump to bottom

⚠️ Update tests to Ginkgo v2 #6906

Merged
merged 1 commit into from
Aug 22, 2022
Merged

⚠️ Update tests to Ginkgo v2 #6906

merged 1 commit into from
Aug 22, 2022

Conversation

mboersma
Copy link
Contributor

What this PR does / why we need it:

Updates CAPI to Ginkgo v2.

Kubernetes just migrated in kubernetes/kubernetes#109111. I attempted to update Ginkgo in CAPZ but can't because of test framework depencies here in CAPI.

The reporting framework has changed significantly: essentially, custom reporters are out. The --junit-report and related flags should give us similar output files, but let's see.

Which issue(s) this PR fixes:

Fixes #4897

cc: @vladimirvivien

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 12, 2022
@mboersma mboersma mentioned this pull request Jul 12, 2022
Closed
3 tasks
sbueringer
sbueringer reviewed Jul 13, 2022
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@mkumatag mkumatag mentioned this pull request Jul 13, 2022
Closed
fabriziopandini
fabriziopandini reviewed Jul 14, 2022
View reviewed changes
test/e2e/Makefile Outdated Show resolved Hide resolved
@mboersma
Copy link
Contributor Author

/retitle [WIP] Update tests to Ginkgo v2

@k8s-ci-robot k8s-ci-robot changed the title 🌱 Update tests to Ginkgo v2 [WIP] Update tests to Ginkgo v2 Jul 14, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2022
@sedefsavas sedefsavas mentioned this pull request Jul 14, 2022
Closed
@enxebre
Copy link
Member

enxebre commented Jul 18, 2022

I think we should only do this if there is a good reason why we have to use ginkgo v2 in CAPI v1.2

+1, it does not seem to be the case at the moment.

@mboersma
Copy link
Contributor Author

/retitle 🌱 Update tests to Ginkgo v2

@k8s-ci-robot k8s-ci-robot changed the title [WIP] Update tests to Ginkgo v2 🌱 Update tests to Ginkgo v2 Jul 18, 2022
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 18, 2022
@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 19, 2022
@mboersma
Copy link
Contributor Author

/hold

In today's CAPI office hours meeting, we agreed to merge this in a month, because we assume providers are still integrating the v1.2.0 release. I will send an email to the list announcing that timeline so providers have time to prepare for this breaking change.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2022
@CecileRobertMichon
Copy link
Contributor

Looks like it's counting SynchronizedBeforeSuite as a spec in the Junit reporting, not sure if that's expected or not

Screen Shot 2022-07-20 at 3 53 12 PM

@richardcase richardcase mentioned this pull request Jul 27, 2022
Merged
4 tasks
@tobiasgiese tobiasgiese mentioned this pull request Jul 30, 2022
Closed
deepakm-ntnx added a commit to deepakm-ntnx/cluster-api-provider-nutanix that referenced this pull request Aug 10, 2022
@deepakm-ntnx
[WIP] Changes to make use of ginkgo v2
51c05c1 
Ref: kubernetes-sigs/cluster-api#6906
@deepakm-ntnx deepakm-ntnx mentioned this pull request Aug 10, 2022
Merged
@sbueringer
Copy link
Member

I should have time for a review early next week.

@mboersma
Copy link
Contributor Author

/retitle ⚠️ Update tests to Ginkgo v2

@k8s-ci-robot k8s-ci-robot changed the title 🌱 Update tests to Ginkgo v2 ⚠️ Update tests to Ginkgo v2 Aug 22, 2022
killianmuldoon
killianmuldoon approved these changes Aug 22, 2022
View reviewed changes
Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

/lgtm

@mboersma
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 22, 2022
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2022
@mboersma
Copy link
Contributor Author

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-workload-upgrade-1-21-1-22-main
/test pull-cluster-api-e2e-workload-upgrade-1-24-latest-main

@sbueringer
Copy link
Member

Thank you!

lgtm pending green tests

@sbueringer
Copy link
Member

sbueringer commented Aug 22, 2022 •
edited
Loading

I assume otherwise we're basically ready to merge (considering the mail in which we wrote we want to merge today :)).

cc @CecileRobertMichon @fabriziopandini

@mboersma
Copy link
Contributor Author

Looks like it's counting SynchronizedBeforeSuite as a spec in the Junit reporting, not sure if that's expected or not

@CecileRobertMichon I see that changed, but I haven't been able to track down where and why. I assume it's part of the new --junit-report implementation. It seems benign, but I can look into it more deeply if we don't like this behavior.

@sbueringer
Copy link
Member

Looks like it's counting SynchronizedBeforeSuite as a spec in the Junit reporting, not sure if that's expected or not

@CecileRobertMichon I see that changed, but I haven't been able to track down where and why. I assume it's part of the new --junit-report implementation. It seems benign, but I can look into it more deeply if we don't like this behavior.

It would be interesting to know, but definitely not a showstopper for me.

@sbueringer
Copy link
Member

/lgtm

CecileRobertMichon
CecileRobertMichon approved these changes Aug 22, 2022
View reviewed changes
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold

feel free to remove the hold when we're ready to merge

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 22, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 Aug 22, 2022
@CecileRobertMichon
Copy link
Contributor

I see that changed, but I haven't been able to track down where and why. I assume it's part of the new --junit-report implementation. It seems benign, but I can look into it more deeply if we don't like this behavior.

definitely not a blocker, just seemed interesting

@mboersma
Copy link
Contributor Author

/hold cancel

I think this is ready to merge. Thanks everyone for the reviews and the help!

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 22, 2022
@k8s-ci-robot k8s-ci-robot merged commit fa70f61 into kubernetes-sigs:main Aug 22, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.3 milestone Aug 22, 2022
@mboersma mboersma deleted the ginkgo-v2 branch August 22, 2022 21:37
@sbueringer
Copy link
Member

@mboersma Thank you very much for pushing this forward! :)

deepakm-ntnx added a commit to nutanix-cloud-native/cluster-api-provider-nutanix that referenced this pull request Aug 29, 2022
@deepakm-ntnx
Changes to make use of ginkgo v2 (#105)
4176c1e 
* [WIP] Changes to make use of ginkgo v2

Ref: kubernetes-sigs/cluster-api#6906

* KRBN-5429 added validation for machine template parameters

* fixes for supporting ginkgo v2

* fixed panic by calling setupsinglehandler only once

* updated package with cve fixes
deepakm-ntnx added a commit to deepakm-ntnx/cluster-api-provider-nutanix that referenced this pull request Aug 29, 2022
@deepakm-ntnx
Changes to make use of ginkgo v2 (nutanix-cloud-native#105)
aff44bc 
* [WIP] Changes to make use of ginkgo v2

Ref: kubernetes-sigs/cluster-api#6906

* KRBN-5429 added validation for machine template parameters

* fixes for supporting ginkgo v2

* fixed panic by calling setupsinglehandler only once

* updated package with cve fixes
deepakm-ntnx added a commit to nutanix-cloud-native/cluster-api-provider-nutanix that referenced this pull request Aug 30, 2022
@deepakm-ntnx
Enabled k8s conformance test (#118)
79ed097 
* [WIP] Changes to make use of ginkgo v2

Ref: kubernetes-sigs/cluster-api#6906

* KRBN-5429 added validation for machine template parameters

* fixes for supporting ginkgo v2

* fixed panic by calling setupsinglehandler only once

* updated package with cve fixes

* Added clusterctl upgrades related fixes

* added proper contracts for each providers for upgrade to work

* fixed metadata.yaml

* corrected versions

* KRBN-5429 added validation for machine template parameters

* removed changed from other pr

* removed changes from other PR

* added more labels

* removed fail-fast so that ginkgo runs all the tests in CI

* added pure conformance test for capx

* updated labels

* added nutanix copyright

* added TODO comment and copyright

* added correct labels
@furkatgofurov7 furkatgofurov7 mentioned this pull request Nov 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackfrancis jackfrancis jackfrancis left review comments

@sbueringer sbueringer sbueringer left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@CecileRobertMichon CecileRobertMichon CecileRobertMichon approved these changes

@killianmuldoon killianmuldoon killianmuldoon approved these changes

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

Assignees

@schrej schrej

@sbueringer sbueringer

@CecileRobertMichon CecileRobertMichon

@killianmuldoon killianmuldoon

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
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

Replatform e2e on to ginkgo v2
9 participants
@mboersma @enxebre @CecileRobertMichon @sbueringer @schrej @k8s-ci-robot @jackfrancis @fabriziopandini @killianmuldoon