-
Notifications
You must be signed in to change notification settings - Fork 114
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
[6.15.z][Combined Jenkins Ask] Capsule testing for sanity (#15948) #16420
Conversation
trigger: test-robottelo |
PRT Result
|
* Capsule Sanity Test from installer * Design Change: only count change * Satellite Maintain fapolicyd package installation
trigger: test-robottelo |
deploy_args = get_deploy_args(request) | ||
with Broker(**deploy_args, host_class=Satellite, _count=2) as hosts: | ||
yield hosts | ||
if 'build_sanity' not in request.config.option.markexpr: |
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.
@jyejare This is not a good approach. This change will cause unnecessary call to module_target_sat
and extra checkout in normal automation run.
if satellite.execute('rpm -q satellite-maintain').status == 0: | ||
# Installing the rpm on existing sat needs sat-maintain perms | ||
cmmd = 'satellite-maintain packages install -y fapolicyd' |
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 is wrong, there's won't be any satellite related packages installed on the rhel machine. It has to be done through dnf. Satellite(including satellite-maintain) packages install is done in later steps.
if satellite.execute('rpm -q satellite-maintain').status == 0: | |
# Installing the rpm on existing sat needs sat-maintain perms | |
cmmd = 'satellite-maintain packages install -y fapolicyd' |
@@ -1381,8 +1383,8 @@ def sat_default_install(module_sat_ready_rhels): | |||
'scenario satellite', | |||
f'foreman-initial-admin-password {settings.server.admin_password}', | |||
] | |||
install_satellite(module_sat_ready_rhels[0], installer_args) | |||
sat = module_sat_ready_rhels[0] | |||
sat = module_sat_ready_rhels.pop() |
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.
Why are we using pop
here. It'll remove Satellite from module_sat_ready_rhels
, this could be used somewhere later in some other tests.
PRT Result
|
@jameerpathan111 this is a cherry-pick, so should be merged as-is. @jyejare can then address your feedback in a new PR against master, which will then be cherry-picked back on top of this. With that in mind, please collect your feedback and file a new issue with everything. |
Sure |
@jyejare @JacobCallahan As this is a manual cherrypick only, so how about we handle the feedback from @jameerpathan111 here only and open a separate PR to address in master/6.16.z only, that way we could avoid more cherrypick PRs :) |
I'll leave that up to @jyejare |
@Gauravtalreja1 I dont see any immediate blocker for merging this one from Jameers comment. Max there would be an increase in total time for installer tests. |
Problem Statement
Caspule Test isnt a part of sanity testing due to several reasons:
Solution
Adding capsule installer test as part of sanity since Sanity is replacing bats tests in jenkins migration in delivery.
Related Issues