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.

Sign up for GitHub

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

Jump to bottom

Update and fix RHCloud tests part one #16751

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

Update and fix RHCloud tests part one #16751

wants to merge 1 commit into from

Conversation

ColeHiggins2
Copy link
Member

Test fixes for RHCloud.

  1. Reversing the changes made here due to failed package counts. Adding a manual insights register after host is registered.
  2. Adding logic to ensure that the proper host status is being pulled and not just what is in index[0]

@ColeHiggins2 ColeHiggins2 added CherryPick PR needs CherryPick to previous branches AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing Stream Introduced in or relating directly to Satellite Stream/Master 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 Oct 23, 2024
@ColeHiggins2 ColeHiggins2 self-assigned this Oct 23, 2024
@ColeHiggins2 ColeHiggins2 requested review from a team as code owners October 23, 2024 19:02
@ColeHiggins2
Copy link
Member Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_rhcloud_inventory.py -k 'test_positive_sync_inventory_status'

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9124
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_rhcloud_inventory.py -k test_positive_sync_inventory_status --external-logging
Test Result : ========= 16 deselected, 625 warnings, 12 errors in 1918.12s (0:31:58) =========

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label Oct 23, 2024
chris1984
chris1984 approved these changes Oct 23, 2024
View reviewed changes
Copy link
Member

@chris1984 chris1984 left a comment

Choose a reason for hiding this comment

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

LGTM, makes sense from what we talked about

@ColeHiggins2
Copy link
Member Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_rhcloud_inventory.py -k test_positive_sync_inventory_status
env:
ROBOTTELO_server__deploy_arguments__deploy_rhel_version: '8'
ROBOTTELO_server__deploy_arguments__deploy_sat_version: '6.16.0'

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9127
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_rhcloud_inventory.py -k test_positive_sync_inventory_status --external-logging
Test Result : ========= 16 deselected, 59 warnings, 12 errors in 4767.42s (1:19:27) ==========

@ColeHiggins2
Copy link
Member Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_rhcloud_inventory.py -k test_positive_sync_inventory_status
env:
ROBOTTELO_server__deploy_arguments__deploy_sat_version: '6.16.0'
ROBOTTELO_server__deploy_arguments__deploy_snap_version: '8.0'
ROBOTTELO_ui__RECORD_VIDEO: true

@ColeHiggins2
Copy link
Member Author 

trigger: test-robottelo
pytest: tests/foreman/cli/test_rhcloud_inventory.py -k test_positive_sync_inventory_status
env:
  ROBOTTELO_server__deploy_arguments__deploy_rhel_version: '8'
  ROBOTTELO_server__deploy_arguments__deploy_sat_version: '6.16.0'
  ROBOTTELO_server__deploy_arguments__deploy_snap_version: '9.0'

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9163
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_rhcloud_inventory.py -k test_positive_sync_inventory_status --external-logging
Test Result : ========= 12 passed, 16 deselected, 920 warnings in 5403.39s (1:30:03) =========

@Satellite-QE Satellite-QE added PRT-Passed Indicates that latest PRT run is passed for the PR and removed PRT-Failed Indicates that latest PRT run is failed for the PR labels Oct 25, 2024
@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9164
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_rhcloud_inventory.py -k test_positive_sync_inventory_status --external-logging
Test Result : ==== 2 failed, 10 passed, 16 deselected, 911 warnings in 5328.71s (1:28:48) ====

@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 Oct 25, 2024
@ColeHiggins2
Copy link
Member Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_rhcloud_inventory.py -k test_positive_sync_inventory_status
env:
ROBOTTELO_server__deploy_arguments__deploy_rhel_version: '8'
ROBOTTELO_server__deploy_arguments__deploy_sat_version: '6.16.0'
ROBOTTELO_server__deploy_arguments__deploy_snap_version: '9.0'

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9169
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_rhcloud_inventory.py -k test_positive_sync_inventory_status --external-logging
Test Result : ======== 12 passed, 16 deselected, 1162 warnings in 6129.83s (1:42:09) =========

@Satellite-QE Satellite-QE added PRT-Passed Indicates that latest PRT run is passed for the PR and removed PRT-Failed Indicates that latest PRT run is failed for the PR labels Oct 25, 2024
Gauravtalreja1
Gauravtalreja1 reviewed Oct 25, 2024
View reviewed changes
robottelo/hosts.py
Comment on lines +1115 to +1117
# Register client with insights
if self.execute('insights-client --register').status != 0:
raise ContentHostError('Unable to register client with insights')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think manual install and register for insight-client would be required only when register=False, so could this move in if-else?

Copy link
Member Author

Choose a reason for hiding this comment

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

This code is weird. Basically, we register a host in order to create the custom repos needed for insights(these base OS repos are too big to enable and sync every test). Insights client does not register during this. Its only when we install packages and manually register at the end that insights gets set up.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to the workflow, it's dumb but this works.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ColeHiggins2 I don't understand why are we moving the registration part at the start of the configure_insights_client. We probably should remove yum install -y insights-client part here as the insights-client should get install during host registration process, we don't need to do it manually.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since its general helper which install and insights during registration process when register=True , and when register=False we should do these setup manually

Copy link
Contributor

@jameerpathan111 jameerpathan111 Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's probably better to get rid of this helper, it's very old and it's definition and usage are now very confusing. We can keep the creating custom repo part(and rename it accordingly) and then do registration like we normally do using global registration command.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ColeHiggins2 I already suggested the same thing on your last PR #16497 (comment),
But if we consider getting rid of this helper itself, so I'd suggest using repo_data option in global registratio, with this option while registering we could pass custom repos and it creates foreman_register repo in /etc/yum.repos.d/.
For 6.16 and stream, it does support creating multiple repos now, and it creates foreman_register1, foreman_register2 repo files in /etc/yum.repos.d/

jameerpathan111
jameerpathan111 requested changes Oct 29, 2024
View reviewed changes
robottelo/hosts.py
Comment on lines +1115 to +1117
# Register client with insights
if self.execute('insights-client --register').status != 0:
raise ContentHostError('Unable to register client with insights')
Copy link
Contributor

Choose a reason for hiding this comment

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

@ColeHiggins2 I don't understand why are we moving the registration part at the start of the configure_insights_client. We probably should remove yum install -y insights-client part here as the insights-client should get install during host registration process, we don't need to do it manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gauravtalreja1 Gauravtalreja1 Gauravtalreja1 left review comments

@jameerpathan111 jameerpathan111 jameerpathan111 requested changes

@chris1984 chris1984 chris1984 approved these changes

Assignees

@ColeHiggins2 ColeHiggins2

Labels
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-Passed Indicates that latest PRT run is passed for the PR Stream Introduced in or relating directly to Satellite Stream/Master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ColeHiggins2 @Satellite-QE @chris1984 @jameerpathan111 @Gauravtalreja1