Skip to content
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 target os name due to RHELC-1737 #17208

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shweta83
Copy link
Contributor

Problem Statement

Bug#RHELC-1737: Incorrect target os name post conversion to RHEL

Solution

Updated assertion

Related Issues

@shweta83 shweta83 added CherryPick PR needs CherryPick to previous branches AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing 6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 6.16.z Introduced in or relating directly to Satellite 6.16 labels Dec 19, 2024
@shweta83 shweta83 requested review from a team as code owners December 19, 2024 11:34
@shweta83 shweta83 force-pushed the skip_rhel_os_check branch 2 times, most recently from 1bf68ea to e24e014 Compare December 19, 2024 11:43
@shweta83
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/api/test_convert2rhel.py -k test_convert2rhel_centos_with_pre_conversion_template_check[centos8]

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9680
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/api/test_convert2rhel.py -k test_convert2rhel_centos_with_pre_conversion_template_check[centos8] --external-logging
Test Result : ========== 1 passed, 3 deselected, 16 warnings in 1759.55s (0:29:19) ===========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Dec 19, 2024
@shweta83
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/api/test_convert2rhel.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9681
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/api/test_convert2rhel.py --external-logging
Test Result : ============ 3 passed, 772 warnings, 1 error in 6235.11s (1:43:55) =============

@Satellite-QE Satellite-QE added PRT-Failed Indicates that latest PRT run is failed for the PR and removed PRT-Passed Indicates that latest PRT run is passed for the PR labels Dec 20, 2024
Comment on lines 432 to 436
assert (
"CentOS"
if is_open('RHELC-1737')
else target_os_name in convert2rhel_facts['conversions.target_os.name']
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't good practice to modify assertion as per open issue, but when checking locally this assertion is always true, so I'd suggest to handle it in the variable instead of assertion, and I think it's best to check 'CentOS Linux' instead of just CentOS for target_os.name

Suggested change
assert (
"CentOS"
if is_open('RHELC-1737')
else target_os_name in convert2rhel_facts['conversions.target_os.name']
)
target_os_name = 'CentOS Linux' if is_open('RHELC-1737') else 'Red Hat'
assert target_os_name in convert2rhel_facts['conversions.target_os.name']

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is altered because of the open bug and I added a check for it. But you are right, we should not do incorrect assertions. I have skipped the assertion until bug is fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, I don't mean to skip entire assertion until bug is fixed, instead we can check if we're getting same target OS as source OS until its fixed as suggested in the target_os_name variable and not in assertion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should we assert the incorrect target_os_version? I think it is better to skip it. We do not expect os_version to be CentOS or Oracle post conversion. Wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we've been validating this fact for a while for conversion tests, so I don't think it's good idea to skip it now, instead I'd suggest and think asserting both source_os and target_os is good addition to our conversion tests, and anyway It'll verify at least we're getting this facts after conversion

@shweta83
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/api/test_convert2rhel.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9694
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/api/test_convert2rhel.py --external-logging
Test Result : ======= 2 failed, 1 passed, 718 warnings, 1 error in 4975.29s (1:22:55) ========

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9699
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/api/test_convert2rhel.py --external-logging
Test Result : ======= 2 failed, 1 passed, 699 warnings, 1 error in 4992.45s (1:23:12) ========

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 6.16.z Introduced in or relating directly to Satellite 6.16 AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing CherryPick PR needs CherryPick to previous branches PRT-Failed Indicates that latest PRT run is failed for the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants