Rewrite the CLI module using OO, and organize argparse #3175

renanrodrigo · 2024-06-19T04:46:42Z

Why is this needed?

This PR solves all of our problems because this will set a new standard for the Pro Client CLI, which will be leveraged for the new UX implementation.

Implement commands and arguments as classes
Simplify separation of concerns, maintainability and testing
Split the CLI into submodules
Test help text in behave

Test Steps

~~This PR should contain only refactors.~~ Yep check tox and behave, small changes to help text

(un)check this to re-run the checklist action

github-actions · 2024-06-19T04:46:55Z

PR Checklist

How to use this checklist

PR Author

For each section, check a box when it is true.
Uncheck a box if it becomes un-true.
Then check the box at the bottom of the PR description to re-run the action that creates this checklist.
The action that creates and updates this comment will retain your edits.
The action will fail if the checklist is not completed.

PR Reviewer

Check that the PR checklist action did not fail.
Double check that the author filled out the checklist accurately.
If you disagree with a checklist item, start a conversation.
For example, the author may say they don't think integration tests are necessary, but you may disagree.

Bug References

None.

Confirm

I've properly referenced all bugs that this PR fixes

How to properly reference fixed bugs

If this PR is related to a Jira item, include an SC-1234 reference in the PR title
If this PR is fixes a GitHub issue, include a Fixes: #1234 reference in the commit that fixes the issue
If this PR is fixes a Launchpad bug, include a LP: #12345678 reference in the commit that fixes the issue

Test Updates

Unit Tests

I have updated or added any unit tests accordingly
No unit test changes are necessary for this change

Integration Tests

I have updated or added any integration tests accordingly
No integration test changes are necessary for this change

Documentation

Changes here need to be documented and I have referenced the docs PR in the description
No documentation updates are necessary for this change

Does this PR require review from someone outside the core ubuntu-pro-client team?

Yes, and I have requested those reviews via GitHub
No

renanrodrigo · 2024-06-19T05:32:02Z

@lucasmoura @orndorffgrant @dheyay I want to do this to ALL commands but please take preliminary looks to make sure you agree with the structure - easier to change earlier when less commands are implemented/affected

orndorffgrant

👏 👏 👏 Excellent - big fan

How much should we convert to this at once? Should we just go for all of it? (each command in a separate commit, though)

uaclient/cli/commands.py

dheyay

Looks great, will make the CLI module readable 👀 🎈

(Should have just left this as a comment, since this is WIP)

renanrodrigo · 2024-06-26T12:10:00Z

@orndorffgrant I want to do this to all commands, yes - maybe incrementally, simple ones in this PR, if I find something more tricky (and I will) I can send it separate for ease of review. As it is a refactor anyway, we can merge without usability changes

orndorffgrant · 2024-06-26T13:46:38Z

@orndorffgrant I want to do this to all commands, yes - maybe incrementally, simple ones in this PR, if I find something more tricky (and I will) I can send it separate for ease of review. As it is a refactor anyway, we can merge without usability changes

@renanrodrigo makes sense. Maybe can you prioritize the subcommands that have their own module? The we'd only have "new" commands in this form and "old" commands in cli/__init__.py (and no "in-between" commands in the better-but-not-as-good separate module form)

renanrodrigo · 2024-06-26T13:51:20Z

@renanrodrigo makes sense. Maybe can you prioritize the subcommands that have their own module? The we'd only have "new" commands in this form and "old" commands in cli/__init__.py (and no "in-between" commands in the better-but-not-as-good separate module form)

Not sure - some of those have been separated exactly because they are more complicated somehow. I'll try and see what happens

Those classes will shape our CLI entrypoints with some OO so we write less boilerplate code and organize things better. Commands and options will be instances of those classes. Signed-off-by: Renan Rodrigo <[email protected]>

Signed-off-by: Renan Rodrigo <[email protected]>

we want to ensure long_name and help, but anything else just passes by. Signed-off-by: Renan Rodrigo <[email protected]>

Signed-off-by: Renan Rodrigo <[email protected]>

uaclient/cli/commands.py

Grouping arguments enables us to create logical groups of arguments when dealing with help text in the future, and to create mutually exclusive groups - which are implemented in the code in one way or the other - and need to be standardized.

renanrodrigo · 2024-07-12T17:14:23Z

@lucasmoura @orndorffgrant @dheyay this is now ready for review. Please keep in mind:

the help text is standardized and broken - next PR will target only help outputs
there are some workarounds that I could improve, but not remove.
I could rewrite a lot more, but focused on standardizing and organizing argparse magic.

Signed-off-by: Renan Rodrigo <[email protected]>

There is no need to create a whole new action for this using the new approach to the CLI. Furthermore, we already do some arg surgery, and this makes parse_args return immmeditely without further burden. And it does show the version. Signed-off-by: Renan Rodrigo <[email protected]>

Signed-off-by: Renan Rodrigo <[email protected]>

And the parser is now ProArgumentParser Signed-off-by: Renan Rodrigo <[email protected]>

orndorffgrant

Approving with the understanding that we need at least the most basic form of the follow up --help PR merged. Specifically, the usage line for sub-subcommands (e.g. pro config show) is broken after this PR.

And a nit that doesn't block this PR: I'd like to change the help behave tests to test stdout equality rather than using a regexp. We can put "optional arguments" vs "options" into an example table variable per ubuntu release.

renanrodrigo requested review from orndorffgrant, dheyay and lucasmoura June 19, 2024 05:32

renanrodrigo force-pushed the object-cli branch 2 times, most recently from 4a10217 to dfe6305 Compare June 19, 2024 05:52

orndorffgrant reviewed Jun 24, 2024

View reviewed changes

uaclient/cli/commands.py Outdated Show resolved Hide resolved

dheyay approved these changes Jun 25, 2024

View reviewed changes

renanrodrigo force-pushed the object-cli branch 5 times, most recently from edd90dd to d127c36 Compare July 3, 2024 19:06

renanrodrigo added 9 commits July 3, 2024 16:08

cli: add ProCommand and ProArgument classes

6e442fa

Those classes will shape our CLI entrypoints with some OO so we write less boilerplate code and organize things better. Commands and options will be instances of those classes. Signed-off-by: Renan Rodrigo <[email protected]>

cli: refactor collect-logs to the new approach

c0d8279

Signed-off-by: Renan Rodrigo <[email protected]>

cli: pass kwargs to CLI arguments

51f718e

we want to ensure long_name and help, but anything else just passes by. Signed-off-by: Renan Rodrigo <[email protected]>

cli: refactor cli_api to the new approach

8c9e638

Signed-off-by: Renan Rodrigo <[email protected]>

cli: refactor disable to the new approach

7d438d5

Signed-off-by: Renan Rodrigo <[email protected]>

cli: refactor enable to the new approach

fd66ba4

cli: refactor attach to the new approach

8c8a204

Signed-off-by: Renan Rodrigo <[email protected]>

cli: refactor auto-attach to the new approach

f66f306

Signed-off-by: Renan Rodrigo <[email protected]>

cli: refactor detach to the new approach

44ce630

Signed-off-by: Renan Rodrigo <[email protected]>

renanrodrigo force-pushed the object-cli branch from d127c36 to 44ce630 Compare July 3, 2024 19:08

orndorffgrant reviewed Jul 5, 2024

View reviewed changes

uaclient/cli/commands.py Outdated Show resolved Hide resolved

renanrodrigo force-pushed the object-cli branch from 4b91fd8 to 92edb49 Compare July 5, 2024 18:50

renanrodrigo force-pushed the object-cli branch 4 times, most recently from 70477cf to c230f7a Compare July 12, 2024 17:10

renanrodrigo marked this pull request as ready for review July 12, 2024 17:11

renanrodrigo changed the title ~~WIP: CLI refactor~~ CLI refactor Jul 12, 2024

renanrodrigo changed the title ~~CLI refactor~~ Rewrite the CLI module using OO, and organizing argparse Jul 12, 2024

renanrodrigo changed the title ~~Rewrite the CLI module using OO, and organizing argparse~~ Rewrite the CLI module using OO, and organize argparse Jul 12, 2024

renanrodrigo added 7 commits July 13, 2024 09:21

cli: refactor security-status to the new approach

c63a7e5

Signed-off-by: Renan Rodrigo <[email protected]>

cli: refactor fix to the new approach

d8a8de3

Signed-off-by: Renan Rodrigo <[email protected]>

cli: refactor status to the new approach

be1645b

Signed-off-by: Renan Rodrigo <[email protected]>

cli: refactor refresh to the new approach

ce23c54

Signed-off-by: Renan Rodrigo <[email protected]>

cli: refactor help to the new approach

9a8df42

cli: add subcommand functionality to commands

fec8fec

Signed-off-by: Renan Rodrigo <[email protected]>

renanrodrigo force-pushed the object-cli branch 2 times, most recently from e74418e to 61fbe4d Compare July 13, 2024 14:53

cli: refactor system to the new approach

4d19517

Signed-off-by: Renan Rodrigo <[email protected]>

renanrodrigo force-pushed the object-cli branch from 61fbe4d to d205870 Compare July 14, 2024 01:17

renanrodrigo added 2 commits July 14, 2024 23:47

cli: refactor config to the new approach

ae9d39e

Signed-off-by: Renan Rodrigo <[email protected]>

cli: add method to get help for any command directly from parser

c368f40

And the parser is now ProArgumentParser Signed-off-by: Renan Rodrigo <[email protected]>

renanrodrigo force-pushed the object-cli branch from d205870 to c368f40 Compare July 15, 2024 02:48

renanrodrigo mentioned this pull request Jul 18, 2024

Adjustments and tweaks to the help text #3206

Merged

1 task

orndorffgrant approved these changes Jul 19, 2024

View reviewed changes

lucasmoura approved these changes Jul 22, 2024

View reviewed changes

orndorffgrant merged commit 48308d3 into next-v34 Jul 22, 2024
20 of 26 checks passed

orndorffgrant deleted the object-cli branch July 22, 2024 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite the CLI module using OO, and organize argparse #3175

Rewrite the CLI module using OO, and organize argparse #3175

renanrodrigo commented Jun 19, 2024 •

edited

Loading

github-actions bot commented Jun 19, 2024 •

edited by renanrodrigo

Loading

How to use this checklist

PR Author

PR Reviewer

renanrodrigo commented Jun 19, 2024

orndorffgrant left a comment

dheyay left a comment •

edited

Loading

renanrodrigo commented Jun 26, 2024

orndorffgrant commented Jun 26, 2024 •

edited

Loading

renanrodrigo commented Jun 26, 2024

renanrodrigo commented Jul 12, 2024

orndorffgrant left a comment

Rewrite the CLI module using OO, and organize argparse #3175

Rewrite the CLI module using OO, and organize argparse #3175

Conversation

renanrodrigo commented Jun 19, 2024 • edited Loading

Why is this needed?

Test Steps

github-actions bot commented Jun 19, 2024 • edited by renanrodrigo Loading

PR Checklist

How to use this checklist

PR Author

PR Reviewer

Bug References

Confirm

Test Updates

Unit Tests

Integration Tests

Documentation

Does this PR require review from someone outside the core ubuntu-pro-client team?

renanrodrigo commented Jun 19, 2024

orndorffgrant left a comment

Choose a reason for hiding this comment

dheyay left a comment • edited Loading

Choose a reason for hiding this comment

renanrodrigo commented Jun 26, 2024

orndorffgrant commented Jun 26, 2024 • edited Loading

renanrodrigo commented Jun 26, 2024

renanrodrigo commented Jul 12, 2024

orndorffgrant left a comment

Choose a reason for hiding this comment

renanrodrigo commented Jun 19, 2024 •

edited

Loading

github-actions bot commented Jun 19, 2024 •

edited by renanrodrigo

Loading

dheyay left a comment •

edited

Loading

orndorffgrant commented Jun 26, 2024 •

edited

Loading