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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃尡 Add e2e basic operations test #1389

Merged
merged 1 commit into from Oct 23, 2023
Merged

馃尡 Add e2e basic operations test #1389

merged 1 commit into from Oct 23, 2023

Conversation

mboukhalfa
Copy link
Member

Adds basic operations e2e test: reboot and power off/on.
Import capm3 e2e test to reuse public functions.

fixes: #1371

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 11, 2023
@mboukhalfa mboukhalfa force-pushed the Add-e2e-basic-ops/mohammed branch 2 times, most recently from 3c5d6fb to e92b019 Compare October 11, 2023 13:14
@mboukhalfa
Copy link
Member Author

/metal3-bmo-e2e-test

@mboukhalfa
Copy link
Member Author

/metal3-bmo-e2e-test

@mboukhalfa
Copy link
Member Author

/metal3-bmo-e2e-test

@mboukhalfa mboukhalfa force-pushed the Add-e2e-basic-ops/mohammed branch 3 times, most recently from f94b916 to 799df61 Compare October 12, 2023 05:38
@mboukhalfa
Copy link
Member Author

/metal3-bmo-e2e-test

@lentzi90
Copy link
Member

/metal3-bmo-e2e-test

lentzi90
lentzi90 reviewed Oct 12, 2023
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

Excellent work @mboukhalfa !
Added a few nits below, and then one concern: I would like to avoid relying on virsh or similar in the tests. This makes it impossible to reuse them in other contexts. Do you think there is some way we could avoid it?

test/e2e/basic_ops_test.go Outdated Show resolved Hide resolved
test/e2e/basic_ops_test.go Outdated Show resolved Hide resolved
test/e2e/basic_ops_test.go Outdated Show resolved Hide resolved
test/e2e/basic_ops_test.go Outdated Show resolved Hide resolved
test/e2e/config/ironic.yaml Outdated Show resolved Hide resolved
test/e2e/basic_ops_test.go Outdated Show resolved Hide resolved
@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2023
@mboukhalfa mboukhalfa force-pushed the Add-e2e-basic-ops/mohammed branch 2 times, most recently from 9501977 to 47dfa79 Compare October 16, 2023 16:01
@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2023
@mboukhalfa
Copy link
Member Author

/metal3-bmo-e2e-test

@mboukhalfa
Copy link
Member Author

/metal3-bmo-e2e-test

@mboukhalfa
Copy link
Member Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

lentzi90
lentzi90 reviewed Oct 17, 2023
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

Awesome!
/approve

test/e2e/config/ironic.yaml Outdated Show resolved Hide resolved
@mboukhalfa
Copy link
Member Author

/metal3-bmo-e2e-test

@mboukhalfa
Copy link
Member Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

lentzi90
lentzi90 reviewed Oct 19, 2023
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2023
kashifest
kashifest reviewed Oct 23, 2023
View reviewed changes
Copy link
Member

@kashifest kashifest left a comment

Choose a reason for hiding this comment

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

/approve
One question in line.
/hold

test/e2e/basic_ops_test.go
}, e2eConfig.GetIntervals(specName, "wait-available")...)

By("setting the reboot annotation and checking that the BMH was rebooted")
capm3_e2e.AnnotateBmh(ctx, clusterProxy.GetClient(), bmh, rebootAnnotation, pointer.String("{\"force\": true}"))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this AnnotateBMH() function reside here and capm3 should import it rather.

Copy link
Member

Choose a reason for hiding this comment

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

That is a good point. It would probably make sense to move some of the e2e code around BMHs from CAPM3 here.
For me it does not matter much when/how we do it though. I'm fine with doing a refactor PR after implementing the tests for example. This also makes it easier to review the new code we add 馃檪

Copy link
Member

Choose a reason for hiding this comment

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

ok with refactor later.
/hold cancel

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 23, 2023
@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest, lentzi90

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

@metal3-io-bot metal3-io-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 23, 2023
@metal3-io-bot metal3-io-bot merged commit 783eb74 into metal3-io:main Oct 23, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lentzi90 lentzi90 lentzi90 left review comments

@kashifest kashifest kashifest left review comments

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm 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
No milestone
Development

Successfully merging this pull request may close these issues.

Add e2e test for basic operations
4 participants
@mboukhalfa @lentzi90 @metal3-io-bot @kashifest