-
Notifications
You must be signed in to change notification settings - Fork 115
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
CC Automation - reclaim-space CLI test #14425
Conversation
trigger: test-robottelo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me. Let's rerun PRT. Should this also be tested with a custom repo that does an immediate sync? Not sure exactly what reclaim space does but in this scenario I assume all it would be removing is metadata and not actual packages?
trigger: test-robottelo |
@Griffin-Sullivan Reclaim_space actually applies to the |
Looks like the actual error is happening when disabling SCA for the entitlement manifest:
Does this need to be an entitlement manifest? If not try with SCA manifest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK pending question and not-blocking comment !
2. hammer repository reclaim-space --id REPOID --organization-id ORGID | ||
|
||
|
||
:expectedresults: Command works as expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have a better expected result! Something like the space is reclaimed or more descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should state here that the space is really reclaimed and at the end, it should be asserted. The fact this passes without any error doesn't mean it works.
) | ||
# Checking that the fail message isn't present. On a success, no message is returned | ||
if output != {}: | ||
assert 'Could not reclaim the repository' not in output[0]['message'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldnt we have some real scenario of reclaiming space rather than this generic check ?
I mean where we could verify some bytes/mbs are reclaimed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my comments are not really blocking
releasever=REPOS['kickstart']['rhel8_aps']['version'], | ||
) | ||
repo = module_target_sat.api.Repository(id=rh_repo_id).read() | ||
repo.sync(timeout=600) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that all of the steps up until this point should be considered a setup. Consider using the already existing fixtures that prepare the repos for you, e.g. custom_synced_repo
and module_repository
@@ -124,3 +124,41 @@ def test_positive_disable_rh_repo_with_basearch(module_target_sat, module_entitl | |||
} | |||
) | |||
assert 'Repository disabled' in disabled_repo[0]['message'] | |||
|
|||
|
|||
def test_positive_reclaim_space(module_target_sat, module_entitlement_manifest_org): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably change the name so it is not too misleading - I believe you do not really test that any space has been reclaimed, since you only run it on an on-demand
-synced repo, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should verify that some space is actually reclaimed. Absence of error message doesn't mean it works.
Related: #14397 |
There's a lot of conversation around here that's valuable, but I'd like to ask if it's reasonable to get keep this test more focused on the actual CLI error that the BZ was originally written against, which is what it tests currently. I've changed the name to be more explicit, so hopefully that makes the purpose of this clear. |
trigger: test-robottelo |
PRT Result
|
trigger: test-robottelo |
PRT Result
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, @sambible can you rebase?
* Add test for reclaim-space bz * Add test for reclaim-space bz * change test name * Change to non-sca org * change test name * Change to non-sca org
Test for following BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2164997
Also adds the associated CLI endpoint to enable the test.
trigger: test-robottelo
pytest: tests/foreman/cli/test_repositories.py -k 'test_positive_reclaim_space'