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

[1.28] Backport the unified syspurpose command #2745

Merged

Conversation

ptoscano
Copy link
Contributor

@ptoscano ptoscano commented Aug 19, 2021

This is a backport of various commits from main to bring the unified syspurpose subcommand to subscription-manager 1.28.x.

This includes a number of work to:

  • add a basic syspurpose command
  • switch from optparse to argparse, needed for using subparsers
  • move the system purpose subcommands to syspurpose, leaving their top-level versions as deprecated

including all the followup fixes needed for each.

The PRs are, in order of their cherry-pick:

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2000883
Card ID: ENT-4303

@ptoscano ptoscano force-pushed the ptoscano/unified-syspurpose-1.28 branch 4 times, most recently from 97ffeb6 to 7cfa22c Compare August 23, 2021 17:02
@jirihnidek
Copy link
Contributor

@ptoscano BTW: can you please write some summary about missing parts in this PR?

@ptoscano ptoscano force-pushed the ptoscano/unified-syspurpose-1.28 branch from 7cfa22c to 65a713b Compare September 3, 2021 10:00
@ptoscano ptoscano marked this pull request as ready for review September 3, 2021 10:00
jirihnidek and others added 7 commits September 3, 2021 15:31
* Added new syspurpose command
* It has only --show option at this moment
* When system is registered, then the sub-man tries to get
  consumer's syspurpose valyes and then do three-way merge
* When system is not registered, then it is possible only
  display values stored in /etc/rhsm/syspurpose/syspurpose.json
* When no options is provided, then current syspurpose values
  are displayed too
* Added unit tests for new command
* Modified a little bit behavior of other syspurpose commands
  (role, usage, addons, service-level). When the system is
  not registered, then --show option print current syspurpose
  value stored in /etc/rhsm/syspurpose/syspurpose.json

(cherry picked from commit c677cee)
(cherry picked from commit eef1bb1)

PT: the changes to the all the Command classes were done in managercli.
Since we recently switched from optparse to argparse, we need to extract
the translatable messages in Python's argparse, rather than optparse.

Followup of commit eef1bb1.

(cherry picked from commit e2d6a55)
Adapt the sed regexp scraping the help text of config after the switch
from optparse to argparse. In particular, the help text for options with
arguments changed from "--foo=VALUE" to "--foo VALUE".

(cherry picked from commit 45526cd)
With the old optparse implementation, there was a single callback to
fill 'repo_actions' with the various values of the --enable & --disable
options, in the same order they were specified on the command line.
Commit eef1bb1 switched to argparse,
and switched to different destination lists for --enable & --disable,
mixing them back to a single list with first all the repos to enabled
and then all the repos to disable. As result, the order of intermixed
--enable & --disable was lost.

To revert back to the wanted behaviour, implement a custom
argparse.Action for --enable & --disable that appends all the actions to
the same list in the same order they were specified. This removes the
need for post-processing later on.

Card-ID: ENT-4146
(cherry picked from commit 8ac4b0f)
They have always been processed in the same order they were specified,
however this behaviour has never been explicitly noted. Since this
behaviour is wanted, mention it explicitly for good.

(cherry picked from commit ef10c18)
@ptoscano ptoscano force-pushed the ptoscano/unified-syspurpose-1.28 branch from 65a713b to 75528bb Compare September 3, 2021 13:31
m-horky and others added 8 commits September 3, 2021 15:45
* BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1983144
* Card ID: ENT-4233

When unknown subcommand or argument is supplied, an error message is
printed and sub-man exits. This commit changes the error message to be
the same as it was before we switched from optparse to argparse.

(cherry picked from commit 8075c66)
According to the documentation, ArgumentParser.parse_known_args() seems
to work without the command name as first argument. Since CliCommand is
parsing the parameters of a specific command, we can drop the command
itself in addition to the application name before calling
ArgumentParser.parse_known_args(); this avoids the need to drop it after
parsing.

In addition, tweak the check for missing test arguments: 'if not foo' is
also matched by an empty list, which now is a valid list of arguments to
pass for tests. Hence, change it to check for None explicitly.

Adapt the tests accordingly.

(cherry picked from commit d765110)
By default, a '--show' option in ArgumentParser is saved as a namespace
variable 'show'. While it is currently not a problem for the '--show'
option of the 'syspurpose' command, it will be a problem when adding the
options of the other syspurpose subcommands as subparsers of the
'syspurpose' command.

Hence, bind the ArgumentParser option result to a less generic variable
name to avoid clashes.

(cherry picked from commit 2fe0df6)
As an effort to consolidate the handling of all the syspurpose
attributes in a single place, make sure that all the syspurpose
attributes are handled as subcommands of the 'syspurpose' command
(leaving the old commands as deprecated).

Make AbstractSyspurposeCommand able to override the ArgumentParser
created by AbstractCLICommand (its grandparent class), so it is possible
to create a subparser instead of a standalone parser.

Then, make SyspurposeCommand create all the syspurpose subcommands, with
their parsers created as subparsers of it; properly dispatch the parsed
command line options to the right subcommand when SyspurposeCommand is
executed.

Finally, change the short description of each subcommand to say that is
deprecated when shown in the list of the subscription-command commands.

(cherry picked from commit 5406865)
Create a custom 'prog' placeholder for subparsers of the 'syspurpose'
command: this way, they will not have the usage string of the
'syspurpose' command (which includes a '[OPTIONS]' help marker).

(cherry picked from commit d94fd30)
Use a custom usage string for the 'syspurpose' command, so we can
mention that this command may take a submodule to use.

(cherry picked from commit 40f2e9f)
* Card ID: ENT-4013
* BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1974641

After the syspurpose was added as subcommand in 5406865 the 'prev'
variable was not always set correctly. For the first argument the
completion worked fine, but for the second (and further) the last
argument was picked up instead of the last subcommand.

This change iterates over the CLI arguments and uses the last word that
does not start with a dash, skipping all arguments and finding the last
subcommand.

(cherry picked from commit 192e98b)
* Card ID: ENT-4150

Syspurpose was added as subscription-manager subcommand in 5406865.
Sections describing its subcommands were altered to match the update,
but 'syspurpose' itself was not added to the list of subcommands.

(cherry picked from commit 58074eb)
@ptoscano ptoscano force-pushed the ptoscano/unified-syspurpose-1.28 branch from 75528bb to de94e92 Compare September 3, 2021 13:45
Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

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

Tested probably all scenarios and everything works as expected. The code and the list of commits is OK too. 👍

@jirihnidek jirihnidek merged commit 4e29138 into subscription-manager-1.28 Sep 3, 2021
@ptoscano ptoscano deleted the ptoscano/unified-syspurpose-1.28 branch September 3, 2021 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants