-
Notifications
You must be signed in to change notification settings - Fork 115
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
fixes in capsule upgrade playbook tests #10173
Conversation
875979e
to
f3f593d
Compare
@pondrejk Could you add some background of the issue in description to know the issue for reviewers. |
f3f593d
to
67cc7c5
Compare
trigger: test-robottelo |
67cc7c5
to
58f1ecf
Compare
trigger: test-robottelo |
58f1ecf
to
ab3daf9
Compare
trigger: test-robottelo |
@jyejare @Gauravtalreja1 mind revisiting? |
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.
@pondrejk LGTM, Thanks for the updates, few suggestions
f'{target_sat.url}/api/job_invocations/{job.id}/hosts/{sat.id}', | ||
auth=(target_sat.username, target_sat.password), | ||
f'{target_sat.url}/api/job_invocations/{job.id}/hosts/{host[0].id}', | ||
auth=(settings.server.admin_username, settings.server.admin_password), |
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.
You can use get_credentials
func from config to fetch this
ab3daf9
to
ac1407a
Compare
ac1407a
to
e5886cd
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.
One non-blocking suggestion.
response = client.get( | ||
f'{target_sat.url}/api/job_invocations/{job.id}/hosts/{sat.id}', | ||
auth=(target_sat.username, target_sat.password), | ||
f'{target_sat.url}/api/job_invocations/{job.id}/hosts/{host[0].id}', |
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.
using the following patch, you can delete L72.
f'{target_sat.url}/api/job_invocations/{job.id}/hosts/{host[0].id}', | |
f'{target_sat.url}/api/job_invocations/{job.id}/hosts/{target_sat.nailgun_host.id}', |
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.
hm, for some reason this didn't work for me when I was trying that out locally
fixing some test failures in tests related to capsule upgrade playbook -- the capsule target version is no longer hardcoded, incorect hostname and credentials selection is also fixed