-
Notifications
You must be signed in to change notification settings - Fork 83
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
Fixes #37993 - Update cvpurge count to a better description #968
Conversation
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.
Hi Chris! I was able to verify that the hammer field works properly with a variety of inputs. I could see it still being confusing for users that the number of CVVs remaining is one (or more) more than the "versions to keep" flag they passed in, but I do think the help text does a good job of showing that it's the unused CVVs. Since this isn't changing any of the functionality of this command, I'm totally okay with how purge functions.
The source looks good to me but I'd love if you could split the long source lines instead of disabling the limit.
Wanted to get this down in writing before I forget: We discussed changing the linelength config for rubocop hammer to match rubocop katello because they are so different. Katello is set up for length 200 at the moment. I'm totally okay with these changes and that would let the longer lines here through without modification. |
@qcjames53 how does this look: Help[vagrant@ip-10-0-168-55 hammer-cli-katello]$ hammer content-view purge -h
Usage:
hammer content-view purge [OPTIONS]
Options:
--async Do not wait for the task
--count NUMBER (deprecated) Number of versions to keep
Default: 3
--id VALUE Content View numeric identifier
--name VALUE Content View name
--organization[-id|-label] VALUE Organization name/label/id to search by
--versions-to-keep NUMBER Number of unused versions to keep
Default: 3
-h, --help Print help Command showing the deprecation warning:[vagrant@ip-10-0-168-55 hammer-cli-katello]$ hammer content-view purge --organization-id 1 --id 5 --count 3
The --count option is deprecated and will be removed in the next release.
[.................................................................................................................................................................................................................................................................................................................................................................................] [100%]
Version '1.0' of content view 'Test-Purge' deleted. If that looks good I will push up the changes and fixes to the line length. |
fa82d46
to
9626591
Compare
@qcjames53 updated |
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.
The new help text looks great to me; both options have appropriate descriptions. I was able to trigger the deprecation warning when running --count
. Functionality for both purge options seems to work. The unit tests seem to work well. ACKing :)
--count
parameter was causing confusion amongst users which lead them to believe that is how many versions to purge instead of keep.--versions-to-keep
and updated the options in the code and in the tests.versions_to_purge = old_unused_versions.slice(0, old_unused_versions.size - option_versions_to_keep)
To test
versions-to-keep
the default is 3 it will remove 1 version