-
Notifications
You must be signed in to change notification settings - Fork 247
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
🌱 Implement BMH provisioning E2E test #1403
🌱 Implement BMH provisioning E2E test #1403
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
I'm very excited to get this in, since it means we cover all the most important operations.
test/e2e/basic_provisioning_test.go
Outdated
}, e2eConfig.GetIntervals(specName, "wait-provisioned")...) | ||
|
||
By("Triggering the deprovisioning of the BMH") | ||
bmhPatch = client.MergeFrom(bmh.DeepCopy()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest the same patch helper here
be75aca
to
07e7981
Compare
b4d0777
to
25044fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple more comments. This is progressing really nicely!
/metal3-bmo-e2e-test
/metal3-bmo-e2e-test |
Great job! Please squash and I will put LGTM on it 🙂 (no need to add me as co-author for those small fixes 😅 ) |
I did change the image to the latest CirrOS (I think I took 0.5.2 because of how the MD5SUM file looked and thought that was the latest at first glance). Now I experiment with the export of variables to the conf file from the ci-hack script and got some problems. I will squash the commits after I got this fixed. |
this part: # These variables are used by the tests. They override variables in the config file.
# This IP is defined by the network we created above.
# Together with the VBMC_PORT this becomes the BMC_ADDRESS used by the BMH in the test.
IP_ADDRESS="192.168.222.1"
export BMC_ADDRESS="ipmi://${IP_ADDRESS}:${VBMC_PORT}"
export BOOT_MAC_ADDRESS
# These are the VBMC defaults (used since we did not specify anything else for `vbmc add`).
export BMC_USER=admin
export BMC_PASSWORD=password
IMAGE_FILE="cirros-0.6.2-x86_64-disk.img"
export IMAGE_MD5SUM="c8fc807773e5354afe61636071771906"
export IMAGE_URL="http://${IP_ADDRESS}/${IMAGE_FILE}" After testing it, they do NOT override the variables in the config file. Any thoughts on that @lentzi90 ? Should I take back the function I had earlier for doing that? |
9d23463
to
d7842ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/metal3-bmo-e2e-test |
/test-ubuntu-integration-main |
/cc @kashifest @dtantsur |
d7842ee
to
bb41e6e
Compare
New changes were fixing a mismatch in variable naming between the hack script and e2e config. (It did not fail any tests since we anyway had the same value in both places.) |
/test-ubuntu-integration-main |
/test-centos-e2e-integration-main |
1 similar comment
/test-centos-e2e-integration-main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome, and I think it's a great improvement.
Just some tiny bit comments, otherwise lgtm.
bb41e6e
to
24e3f6e
Compare
7b54276
to
f5302b9
Compare
f5302b9
to
cbc8963
Compare
/metal3-bmo-e2e-test |
cbc8963
to
b15a370
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/metal3-bmo-e2e-test |
/test-ubuntu-integration-main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
Great job on your first PR @maxrantil
One question in line.
/hold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kashifest 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 |
b15a370
to
ad250ef
Compare
ad250ef
to
afa14f6
Compare
/metal3-bmo-e2e-test |
Introduce an end-to-end test validating the provisioning and deprovisioning flow of a BareMetalHost.
afa14f6
to
bde3a84
Compare
/metal3-bmo-e2e-test |
/test-ubuntu-integration-main |
/hold cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Previous test lost contact to jenkins worker... |
Introduce an end-to-end test validating the provisioning and deprovisioning flow of a BareMetalHost.
Fixes #1367