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

Fixes #37993 - Update cvpurge count to a better description #968

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

Conversation

chris1984
Copy link
Member

  • The --count parameter was causing confusion amongst users which lead them to believe that is how many versions to purge instead of keep.
  • Renamed the param to --versions-to-keep and updated the options in the code and in the tests.
  • The code looks like this which determines how many versions to delete versions_to_purge = old_unused_versions.slice(0, old_unused_versions.size - option_versions_to_keep)

To test

  • Create a content view and publish 4 versions
  • If you don't pase in versions-to-keep the default is 3 it will remove 1 version
  • Create a few more published versions of that content view
  • Pass in how many you want to keep and issue the command to make sure it deletes the versions that are unused minus how many you want to keep.

Copy link
Contributor

@qcjames53 qcjames53 left a 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.

lib/hammer_cli_katello/content_view_purge.rb Outdated Show resolved Hide resolved
test/functional/content_view/purge_test.rb Outdated Show resolved Hide resolved
@qcjames53
Copy link
Contributor

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.

@chris1984
Copy link
Member Author

chris1984 commented Nov 18, 2024

@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.

@chris1984
Copy link
Member Author

@qcjames53 updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants