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

Add tests for ansible-tower/AAP integration with Satellite #16555

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

Add tests for ansible-tower/AAP integration with Satellite #16555

wants to merge 1 commit into from
+285 −0

Conversation

Gauravtalreja1
Copy link
Collaborator

Problem Statement

Missing test coverage for ansible-tower/AAP integration with Satellite

Solution

Add tests for ansible-tower/AAP integration with Satellite

Related Issues

@Gauravtalreja1 Gauravtalreja1 added QETestCoverage Issues and PRs relating to a Satellite bug CherryPick PR needs CherryPick to previous branches AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing 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 3, 2024
@Gauravtalreja1 Gauravtalreja1 self-assigned this Oct 3, 2024
@Gauravtalreja1 Gauravtalreja1 requested review from a team as code owners October 3, 2024 08:08
@Gauravtalreja1 Gauravtalreja1 marked this pull request as draft October 3, 2024 08:08
@Gauravtalreja1 Gauravtalreja1 marked this pull request as ready for review October 4, 2024 19:50
@Gauravtalreja1 Gauravtalreja1 force-pushed the ansible-tower-intergration branch 3 times, most recently from 8158669 to 41af678 Compare October 8, 2024 10:23
@Gauravtalreja1
Copy link
Collaborator Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_ansible.py::TestAnsibleAAPIntegration::test_positive_ansible_dynamic_inventory

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 8904
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_ansible.py::TestAnsibleAAPIntegration::test_positive_ansible_dynamic_inventory --external-logging
Test Result : ================= 2 passed, 49 warnings in 1038.89s (0:17:18) ==================

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Oct 8, 2024
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Oct 9, 2024
@Gauravtalreja1
Copy link
Collaborator Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_ansible.py::TestAnsibleAAPIntegration::test_positive_ansible_provisioning_callback
provisioning: true

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 8921
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_ansible.py::TestAnsibleAAPIntegration::test_positive_ansible_provisioning_callback --external-logging --include-onprem-provisioning
Test Result : ================= 2 failed, 111 warnings in 5631.17s (1:33:51) =================

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

@rplevka rplevka left a comment

Choose a reason for hiding this comment

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

This is super cool and i am excited to see the scenario with the on-premise provisioning. I think we could clean this up a bit by moving the most of the stuff to the fixtures so only the bits relevant to the test scenarios remain, but overall, I really like how well readable this is. Great job!

oh and btw, let's get rid of those static timeouts, unless there is a good reason for not using wait-for. Would that make sense?

tests/foreman/cli/test_ansible.py

password = settings.server.admin_password
if auth_type == 'admin':
login = gen_string('alpha')
Copy link
Member

Choose a reason for hiding this comment

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

this feels like something that belongs to the setup phase rather to the body of the test. Could we possible move this to a fixture that would accept role as a parameter and request it by test by passing it the value of auth_type indirectly?

tests/foreman/cli/test_ansible.py
host.update(['location'])

# Find the Satellite credentials in AAP and update it for target_sat.hostname and user credentials
creds_list = aap_client.get(
Copy link
Member

Choose a reason for hiding this comment

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

This one looks like another candidate for a fixture. I see you use the same code in the other test too.

tests/foreman/cli/test_ansible.py
f'/api/v2/inventory_sources/{inv_source_list["results"][0]["id"]}/update/'
)
assert sync_response.ok
sleep(180)
Copy link
Member

Choose a reason for hiding this comment

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

I see you imported wait-for lib. Why not use it here? Use of static timeouts is discouraged.

tests/foreman/cli/test_ansible.py
assert rhel_contenthost.hostname in [host['name'] for host in hosts_list['results']]

@pytest.mark.on_premises_provisioning
@pytest.mark.parametrize('pxe_loader', ['bios', 'uefi'], indirect=True)
Copy link
Member

Choose a reason for hiding this comment

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

is this something where the choice of bootloader actually matters?
are the callback parameters passed to installer via pxelinux or grub cfg, or are they pulled via http by installer later in the process?
if the latter, we could probably drop one of these and stick to a single scenario. If not, then sorry for interruption, resolve my comment and carry on ;)

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 8935
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_ansible.py::TestAnsibleAAPIntegration::test_positive_ansible_provisioning_callback --external-logging --include-onprem-provisioning
Test Result : ================= 2 failed, 149 warnings in 5799.29s (1:36:39) =================

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

@rplevka rplevka rplevka left review comments

Assignees

@Gauravtalreja1 Gauravtalreja1

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-Failed Indicates that latest PRT run is failed for the PR QETestCoverage Issues and PRs relating to a Satellite bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Gauravtalreja1 @Satellite-QE @rplevka