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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions robottelo/hosts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1080,6 +1080,17 @@ def configure_insights_client(
:param register: Whether to register client to insights
:return: None
"""
# Attempt to register host
if register:
if not activation_key:
activation_key = satellite.api.ActivationKey(
content_view=org.default_content_view.id,
environment=org.library.id,
organization=org,
).create()
self.register(
org, None, activation_key.name, satellite, setup_insights=register_insights
)
# Red Hat Insights requires RHEL 6/7/8/9 repo and it is not
# possible to sync the repo during the tests, Adding repo file.
distro_repo_map = {
Expand All @@ -1101,17 +1112,9 @@ def configure_insights_client(
# Ensure insights-client rpm is installed
if self.execute('yum install -y insights-client').status != 0:
raise ContentHostError('Unable to install insights-client rpm')
# attempt to register host
if register:
if not activation_key:
activation_key = satellite.api.ActivationKey(
content_view=org.default_content_view.id,
environment=org.library.id,
organization=org,
).create()
self.register(
org, None, activation_key.name, satellite, setup_insights=register_insights
)
# Register client with insights
if self.execute('insights-client --register').status != 0:
raise ContentHostError('Unable to register client with insights')
Comment on lines +1115 to +1117
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/


def unregister_insights(self):
"""Unregister insights client.
Expand Down
11 changes: 9 additions & 2 deletions tests/foreman/cli/test_rhcloud_inventory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,15 @@ def test_positive_sync_inventory_status(
task_output = module_target_sat.api.ForemanTask().search(
query={'search': f'{inventory_sync_task} and started_at >= "{timestamp}"'}
)
assert task_output[0].output['host_statuses']['sync'] == 2
assert task_output[0].output['host_statuses']['disconnect'] == 0
host_status = None
for task in task_output:
if task.input.get("organization_ids") is None:
continue
if str(task.input.get("organization_ids")[0]) == str(org.id):
host_status = task.output
break
assert host_status['host_statuses']['sync'] == 2
assert host_status['host_statuses']['disconnect'] == 0


@pytest.mark.tier3
Expand Down