-
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
Update Platform tests that have hard coded content host versions #14317
Conversation
trigger: test-robottelo |
PRT Result
|
00951fb
to
d05812d
Compare
trigger: test-robottelo |
PRT Result
|
d05812d
to
e9d6de2
Compare
trigger: test-robottelo |
PRT Result
|
e9d6de2
to
c342257
Compare
trigger: test-robottelo |
2 similar comments
trigger: test-robottelo |
trigger: test-robottelo |
PRT Result
|
pytest_fixtures/core/contenthosts.py
Outdated
deploy_args['deploy_rhel_version'] = deploy_args.get('deploy_rhel_version', '8') | ||
# if 'deploy_rhel_version' is not set, let's default to what's in server.yaml | ||
deploy_args['deploy_rhel_version'] = deploy_args.get( | ||
'deploy_rhel_version', settings.server.version.rhel_version |
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 server version? Why not version from conf/content_host.yaml
? Is it because it's for PIT?
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 guess this is not obvious from the description but this host becomes a Satellite so I opt to use the Satellite os version from the settings file and not content host.
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.
Since this custom_host fixture can be whatever you want let's do your change here and default to content_host.yaml and in my test we'll keep the server.yaml
@@ -638,7 +638,7 @@ def setup_org_for_a_custom_repo(self, options=None): | |||
lce_promoted = cv_info['lifecycle-environments'] | |||
# Promote version to next env | |||
try: | |||
if env_id not in [int(lce['id']) for lce in lce_promoted]: | |||
if (env_id not in [int(lce['id']) for lce in lce_promoted]) or multi_repo: |
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.
Explain in #
comment please
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.
It's not clear, what is multi_repo arg here could you elaborate? and when multi_repo is True, then shouldn't we check that first then the other check?
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.
Can someone from the Phoenix-content group chime in and provide feedback on it?
@vsedmik @ColeHiggins2 @sambible @vijaysawant
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.
Eh, it seems that @Griffin-Sullivan is trying to bypass @vijaysawant's bypass.
Looking at the first bypass PR, my guess is that it was trying to resolve the CVV Sorting Issue, which was later fixed on line 638.
Anyway, reading the docstring I don't think this helper was meant to be called in a loop, not for adding repos. It was for wrapping a repo into a product and shipping it via CV and AK.
If we need to create more custom repos in one custom product, we could extend the helper and pass the repo urls as a list and add them inside the helper.
Also, wouldn't it be easier for you @Griffin-Sullivan use this instead?
def enable_sync_redhat_repo(self, rh_repo, org_id, timeout=1500): |
I mean baseos
and appstream
are not custom repos, you could sync them as RH repos (just use module_sca_manifest_org
instead of module_org
) and ship them through the CV/LCE/AK used in your content_for_client
fixture?
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 was just trying to get around the issue that I explain in the comment. I was trying to make as minimal changes as possible for the test since I'm just trying to convert to using newer rhel versions. I guess in this case I can revert the change and use a different helper like enable_sync_redhat_repo
} | ||
) | ||
rhel_ver = settings.content_host.default_rhel_version | ||
baseos = eval(f'settings.repos.RHEL{rhel_ver}_OS.baseos') |
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.
How about versions < 8 which don't have baseos and upstream?
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.
My thought process was to keep this simple and not support RHEL 7 but I guess a good question would be how long do we need to support rhel 7? Especially in this one scenario that affects only this test.
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 think it's up to you, I just pointed it out.
{'deploy_rhel_version': '8', 'deploy_flavor': 'satqe-ssd.disk.xxxl'}, | ||
{'deploy_rhel_version': '8', 'deploy_flavor': 'satqe-ssd.standard.std'}, | ||
{ | ||
'deploy_rhel_version': settings.server.version.rhel_version, |
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.
Again, consider using conf/content_host.yaml
here and in the following code (I'm not commenting on everything separately)
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.
See above
89b43c9
to
dd1a20b
Compare
dd1a20b
to
6c24115
Compare
trigger: test-robottelo |
PRT Result
|
PRT Result
|
6c24115
to
bd8a91e
Compare
trigger: test-robottelo |
PRT Result
|
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 final non-blocking nitpick, otherwise looks pretty good! 👍
haproxy.install_katello_ca(module_target_sat) | ||
haproxy.register_contenthost(module_org.label, haproxy_ak.name) |
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 just recommend to use global registration
haproxy.install_katello_ca(module_target_sat) | |
haproxy.register_contenthost(module_org.label, haproxy_ak.name) | |
haproxy.register(module_org, None, haproxy_ak.name, module_target_sat) |
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'd say this is outside of the scope of the PR, but if other reviewers agree to change it now I'll update it.
custom_host.execute( | ||
f'dnf -y module enable satellite:el{rhel_major} && dnf -y install satellite' | ||
) |
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.
Can we add a check here and split this module enable part as this is only for EL8?
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.
My comments have been addressed
) (cherry picked from commit de6fc12)
Problem Statement
PIT isn't running some of Platform's tests because they are hard coded to older rhel versions (7 or 8).
Solution
Update tests to be future-proof and run on whatever the test suite decides (ie: use settings file).
Related Issues