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

Mark one RHSSO test for PIT server #17204

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

Mark one RHSSO test for PIT server #17204

wants to merge 1 commit into from
+3 −1

Conversation

lhellebr
Copy link
Contributor

RHSSO needs some packages shipped by RHEL that we expect to change, we should interop test it. The test will probably never run in older z-streams without it. The test is destructive though, it runs an installer. Debate on allowing destructive tests in PIT is in the corresponding PR in our config repo.

@lhellebr lhellebr requested review from a team as code owners December 18, 2024 16:56
@lhellebr lhellebr 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 18, 2024
@lhellebr
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/destructive/test_ldapauthsource.py::test_rhsso_login_using_hammer

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9670
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/destructive/test_ldapauthsource.py::test_rhsso_login_using_hammer --external-logging
Test Result : ================= 1 passed, 17 warnings in 2002.79s (0:33:22) ==================

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Dec 18, 2024
@devendra104
Copy link
Member

trigger: test-robottelo
pytest: tests/foreman/destructive/test_ldapauthsource.py::test_rhsso_login_using_hammer

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9672
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/destructive/test_ldapauthsource.py::test_rhsso_login_using_hammer --external-logging
Test Result : ================= 1 passed, 17 warnings in 1864.63s (0:31:04) ==================

@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Dec 23, 2024
vsedmik
vsedmik approved these changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@vsedmik vsedmik left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one (non-blocking) question.

pytest_fixtures/component/satellite_auth.py
@@ -289,7 +289,8 @@ def auth_data(request, ad_data, ipa_data):
@pytest.fixture(scope='module')
def enroll_configure_rhsso_external_auth(module_target_sat):
"""Enroll the Satellite6 Server to an RHSSO Server."""
module_target_sat.register_to_cdn()
if settings.robottelo.rhel_source == "ga":
Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't seen this in robottelo.yaml in Jenkins (though it's present in robottelo.yaml.template).
Do I assume correctly that we depend purely on the validator here?

Maybe rather question for @rmynar.

Copy link
Collaborator

@Gauravtalreja1 Gauravtalreja1 Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can try available setting settings.robottelo.cdn == true here? Or add missing setting in robottelo.yaml
CC @rmynar

Copy link
Contributor

Choose a reason for hiding this comment

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

I would not say we have to "depend" on validators. But the idea is to maintain validators responsibly (not only in this case) so we can "trust" them.
Relying on default values and modifying the settings only in specific cases seems OK to me. (The rhel_source setting has to be explicitly changed to "internal" usually for interoperability testing - i.e. testing with unreleased RHEL version)

Copy link
Member

Choose a reason for hiding this comment

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

The setting is set in the .template config

robottelo/conf/robottelo.yaml.template

Lines 25 to 27 in b7406c0

# The source of RHEL packages. Can be one of:
# internal, ga (CDN)
RHEL_SOURCE: "ga"
.

And yes, in this case, we rely on Dynaconf validators to populate the setting for us, but since validators run unconditionally every time (now even in Robottelo CI/CQ), I share the same view as @rmynar. However, there is no harm in adding an explicit setting in the YAML config. Anyone, feel free to submit a patch.

rmynar
rmynar approved these changes Jan 10, 2025
View reviewed changes
pytest_fixtures/component/satellite_auth.py
@@ -289,7 +289,8 @@ def auth_data(request, ad_data, ipa_data):
@pytest.fixture(scope='module')
def enroll_configure_rhsso_external_auth(module_target_sat):
"""Enroll the Satellite6 Server to an RHSSO Server."""
module_target_sat.register_to_cdn()
if settings.robottelo.rhel_source == "ga":
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not say we have to "depend" on validators. But the idea is to maintain validators responsibly (not only in this case) so we can "trust" them.
Relying on default values and modifying the settings only in specific cases seems OK to me. (The rhel_source setting has to be explicitly changed to "internal" usually for interoperability testing - i.e. testing with unreleased RHEL version)

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

@ogajduse ogajduse ogajduse left review comments

@rmynar rmynar rmynar approved these changes

@vsedmik vsedmik vsedmik approved these changes

@pondrejk pondrejk pondrejk approved these changes

Assignees

@lhellebr lhellebr

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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lhellebr @Satellite-QE @devendra104 @pondrejk @ogajduse @Gauravtalreja1 @vsedmik @rmynar