-
Notifications
You must be signed in to change notification settings - Fork 254
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
🐛 Fix VM selection in clean-e2e.sh #1537
Conversation
84dcd85
to
258af6a
Compare
/test metal3-bmo-e2e-test-pull |
LGTM |
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! Just one small comment
hack/clean-e2e.sh
Outdated
@@ -4,21 +4,10 @@ REPO_ROOT=$(dirname "${BASH_SOURCE[0]}")/.. | |||
cd "${REPO_ROOT}" || exit 1 | |||
|
|||
minikube delete | |||
docker rm -f vbmc |
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.
Could you add this back? It feels weird to delete sushy-tools here but not vbmc.
In the long run we can move all of it to the other script but for now I think we can keep it here and just fix the bug.
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.
Sure. I removed it since it felt weird to me that we clean it in both places while it may not even be there, but no big issue.
258af6a
to
6b36034
Compare
Signed-off-by: Huy Mai <huy.mai@est.tech>
6b36034
to
6ca6cdf
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
/test metal3-bmo-e2e-test-pull |
/test-centos-e2e-integration-main |
LGTM |
/test metal3-bmo-e2e-test-pull |
done | ||
else | ||
echo "No virtual machines found. Skipping..." | ||
fi | ||
|
||
# Clear vbmc | ||
docker stop vbmc |
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.
whats the harm if we stop it first, why are we removing this? Is there a possibility that it might not exist?
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.
The -f
makes it unnecessary. We remove it no matter if it runs or not
/approve |
[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 |
Seems this did not start... |
Fixes #1531
This PR takes the
tools/bmh_test/clean_local_bmh_test_setup.sh
into use:-e
as many operations could fail, but the script should keep going