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

🌱 test/e2e: Add ClusterClass rollout test, extend MD rollout test #8171

Merged
merged 1 commit into from
Mar 14, 2023
Merged

🌱 test/e2e: Add ClusterClass rollout test, extend MD rollout test #8171

merged 1 commit into from
Mar 14, 2023

Conversation

sbueringer
Copy link
Member

@sbueringer sbueringer commented Feb 24, 2023 •
edited
Loading

Signed-off-by: Stefan Büringer buringerst@vmware.com

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Adds test coverage for: #7731, #7730

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 24, 2023
@sbueringer sbueringer changed the title 🌱 test/e2e: Implement rollout e2e test [WIP] 🌱 test/e2e: Implement rollout e2e test Feb 24, 2023
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 24, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2023
@sbueringer
Copy link
Member Author

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

@sbueringer
Copy link
Member Author

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

@sbueringer
Copy link
Member Author

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

@sbueringer sbueringer changed the title [WIP] 🌱 test/e2e: Implement rollout e2e test 🌱 test/e2e: Add ClusterClass rollout test, extend MD rollout test Mar 7, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 7, 2023
@sbueringer
Copy link
Member Author

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

@sbueringer
Copy link
Member Author

sbueringer commented Mar 7, 2023 •
edited
Loading

Should be ready for review @killianmuldoon @fabriziopandini @ykakarap

For now this PR still contains a commit with the MD rolloutAfter implementation, please ignore during review.

If the MD rolloutAfter PR is hold back longer I would comment out the corresponding part of the test.

/hold
(to drop the MD rolloutAfter commit)

@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 Mar 7, 2023
ykakarap
ykakarap reviewed Mar 8, 2023
View reviewed changes
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.

Reviewed the new tests and they are good 👍🏼

internal/controllers/topology/cluster/desired_state.go Show resolved Hide resolved
internal/contract/controlplane_template.go Outdated Show resolved Hide resolved
internal/contract/bootstrap_config_template.go Outdated Show resolved Hide resolved
internal/contract/infrastructure_machine_template.go Outdated Show resolved Hide resolved
test/e2e/clusterclass_changes.go Outdated Show resolved Hide resolved
test/e2e/clusterclass_rollout.go Show resolved Hide resolved
test/e2e/clusterclass_rollout.go Show resolved Hide resolved
@ykakarap
Copy link
Contributor

ykakarap commented Mar 8, 2023

Will do another the rolloutAfter PR is merged and this is rebased.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2023
@sbueringer
Copy link
Member Author

Findings adressed + resolved conflicts in the cherry-picked rolloutAfter commit

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

@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 Mar 8, 2023
@sbueringer
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2023
@ykakarap ykakarap mentioned this pull request Mar 9, 2023
Closed
19 tasks
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2023
@sbueringer
Copy link
Member Author

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

@ykakarap PR is now rebased, kept separate commits for now for easier reviewing

@sbueringer
Copy link
Member Author

/retest

1 similar comment
@sbueringer
Copy link
Member Author

/retest

@sbueringer
Copy link
Member Author

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

@sbueringer
Copy link
Member Author

/retest

fabriziopandini
fabriziopandini reviewed Mar 9, 2023
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

only one nit from my side, otherwise lgtm
great to see this level of e2e test coverage at this stage of the release cycle

internal/controllers/topology/cluster/desired_state.go Outdated Show resolved Hide resolved
test/e2e/clusterclass_rollout.go Show resolved Hide resolved
test/e2e/clusterclass_rollout.go Show resolved Hide resolved
ykakarap
ykakarap reviewed Mar 10, 2023
View reviewed changes
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.

Just 2 nits. LGTM otherwise.

test/e2e/clusterclass_rollout.go Outdated Show resolved Hide resolved
test/framework/machinedeployment_helpers.go Outdated Show resolved Hide resolved
@sbueringer
test/e2e: Implement ClusterCLass rollout e2e test
64ef9dd 
Signed-off-by: Stefan Büringer buringerst@vmware.com
@sbueringer
Copy link
Member Author

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

@ykakarap
Copy link
Contributor

ykakarap commented Mar 10, 2023 •
edited
Loading

Thank you!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d8553deb629b956ad0479f6fc8b2ae3cded6be32

@fabriziopandini
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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 Mar 13, 2023
@sbueringer
Copy link
Member Author

/retest

This was referenced Mar 14, 2023
Closed
Closed
@k8s-ci-robot k8s-ci-robot merged commit 189c649 into kubernetes-sigs:main Mar 14, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.4 milestone Mar 14, 2023
@sbueringer sbueringer deleted the pr-e2e-propagation branch March 14, 2023 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabriziopandini fabriziopandini fabriziopandini left review comments

@ykakarap ykakarap ykakarap left review comments

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

Assignees

@fabriziopandini fabriziopandini

@ykakarap ykakarap

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
4 participants
@sbueringer @ykakarap @k8s-ci-robot @fabriziopandini