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

policy list should work after associated hg removal #15031

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

pondrejk
Copy link
Contributor

Problem Statement

SAT-19502 describes the issue, https://github.com/theforeman/foreman_openscap/pull/568/files provides the upstream fix

Solution

Extending an existing test to check the policy behavior after hostgroup removal

Related Issues

@pondrejk pondrejk added No-CherryPick PR doesnt need CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master labels May 10, 2024
@pondrejk pondrejk self-assigned this May 10, 2024
@pondrejk pondrejk requested a review from a team as a code owner May 10, 2024 11:26
@pondrejk
Copy link
Contributor Author

"trigger": "test-robottelo"
"pytest": "tests/foreman/cli/test_oscap.py -k test_positive_associate_scap_policy_with_hostgroups"
"theforeman":
    "foreman_openscap": "568"

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6878
Build Status: SUCCESS
PRT Comment: pytest /opt/app-root/src/robottelo/tests/foreman -v -m build_sanity --external-logging
Test Result : ======= 12 passed, 5423 deselected, 5526 warnings in 1685.28s (0:28:05) ========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label May 10, 2024
@pondrejk pondrejk removed the PRT-Passed Indicates that latest PRT run is passed for the PR label May 10, 2024
@pondrejk
Copy link
Contributor Author

Disregard the prt run, the test collection there didn't match my comment, so I killed the job, investigating...

@Griffin-Sullivan
Copy link
Contributor

trigger: test-robottelo
pytest: tests/foreman/cli/test_oscap.py -k test_positive_associate_scap_policy_with_hostgroups
theforeman:
    foreman_openscap: 568

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6879
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_oscap.py -k test_positive_associate_scap_policy_with_hostgroups --external-logging
Test Result : ========== 1 passed, 70 deselected, 79 warnings in 1633.87s (0:27:13) ==========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label May 10, 2024
Comment on lines 623 to 629
try:
module_target_sat.cli.Scappolicy.list()
except CLIReturnCodeError:
pytest.fail("failed to list policies")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we catching the failure?

Copy link
Member

Choose a reason for hiding this comment

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

I believe that plain raising of the exception results in Error junit status. If you properly fail it, you get Failed status.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, CLIReturnCodeError doesn't inherit from AssertionError so the test would result as Error. And unfortunately pytest doesn't have anything like pytest.not_raises() (yet), which would be cool for these cases.

module_target_sat.cli.HostGroup.delete({'id': hostgroup['id']})
# removal of hostgroup shouldn't affect policies
try:
module_target_sat.cli.Scappolicy.list()
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you assert the policies have indeed been listed?

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.

ACK after adressing @lhellebr 's comment

Comment on lines 623 to 629
try:
module_target_sat.cli.Scappolicy.list()
except CLIReturnCodeError:
pytest.fail("failed to list policies")
Copy link
Member

Choose a reason for hiding this comment

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

I believe that plain raising of the exception results in Error junit status. If you properly fail it, you get Failed status.

@pnovotny
Copy link
Contributor

I would also add some assertion like @lhellebr mentioned.
Otherwise LGTM.

@pondrejk
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_oscap.py -k test_positive_associate_scap_policy_with_hostgroups
theforeman:
    foreman_openscap: 572

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7012
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_oscap.py -k test_positive_associate_scap_policy_with_hostgroups --external-logging
Test Result : ========== 70 deselected, 76 warnings, 1 error in 1425.38s (0:23:45) ===========

@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 May 20, 2024
@pondrejk
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_oscap.py -k test_positive_associate_scap_policy_with_hostgroups
theforeman:
    foreman_openscap: 572

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7015
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_oscap.py -k test_positive_associate_scap_policy_with_hostgroups --external-logging
Test Result : ========== 70 deselected, 76 warnings, 1 error in 1247.67s (0:20:47) ===========

@pondrejk
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_oscap.py -k test_positive_associate_scap_policy_with_hostgroups

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7124
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_oscap.py -k test_positive_associate_scap_policy_with_hostgroups --external-logging
Test Result : =========== 70 deselected, 78 warnings, 1 error in 717.56s (0:11:57) ===========


:expectedresults: The policy is created and associated successfully.
Policies can be listed after hostgroup removal.

:bz: 1728157
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:bz: 1728157
:bz: 1728157
:Verifies: SAT-19502

@pondrejk As we're moving away from Bugzilla, could we also add Jira issue here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

Copy link
Member

@ogajduse ogajduse left a comment

Choose a reason for hiding this comment

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

ACK, one minor comment left.

co-reviewed with @jameerpathan111

# removal of hostgroup shouldn't affect policies
try:
result = module_target_sat.cli.Scappolicy.list()
assert name in [policy['name'] for policy in result]
Copy link
Member

@ogajduse ogajduse Jun 5, 2024

Choose a reason for hiding this comment

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

Minor one: since CLIReturnCodeError can not be raised here, you can move this line below the except block.
Edit: commenting on line assert name in [policy['name'] for policy in result]

@pondrejk
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_oscap.py -k test_positive_associate_scap_policy_with_hostgroups

1 similar comment
@pondrejk
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_oscap.py -k test_positive_associate_scap_policy_with_hostgroups

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7357
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_oscap.py -k test_positive_associate_scap_policy_with_hostgroups --external-logging
Test Result : ========== 1 passed, 70 deselected, 79 warnings in 651.18s (0:10:51) ===========

@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 Jun 11, 2024
@pondrejk pondrejk merged commit ca9193d into SatelliteQE:master Jun 11, 2024
11 checks passed
jyejare pushed a commit to jyejare/robottelo that referenced this pull request Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No-CherryPick PR doesnt need 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
Development

Successfully merging this pull request may close these issues.

8 participants