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

Minder CLI improvements - table interface, refactor use of flags, context, etc. #1919

Merged
merged 12 commits into from
Dec 19, 2023

Conversation

rdimitrov
Copy link
Member

@rdimitrov rdimitrov commented Dec 13, 2023

The following PR:

  • Leverages the use of PersistentFlags and FlagGroups. This resulted in a lot of housekeeping on helper functions we don't really had to have since they are already provided by cobra/viper. Essentially there a no major code logic changes but because of this cleanup the PR looks way bigger than I expected.
  • Refactors the use of tables within the cli. Adds a Table interface with two supported tables (Simple and Glossy). Currently everything is migrated to the Simple one for consistency, but the glossy one is left for eventual PoC since it looks much better, but lacks the appropriate features (or we don't know how to implement them yet).
  • Adds provider and project values (I have started this earlier so I haven't rebased the latest context changes, will do though that's why it's WIP)
  • Went through the CLI stack of commands and made the codebase consistent, refer to the same default values, etc.
  • Fixed an issue where errors were showing twice
  • Went through all cli commands and made sure they use the cli.MessageAndReturn function. Updated the function to wrap a custom error type we later unwrap in ExitNicely so we can handle message outputs better.
  • Reduced the details input of an GRPC error to description only
  • Renamed --repo to name when doing repo register for consistency with the rest of the repo commands
  • Added a new context for each batched profile call
  • Converged on using the basic table rendering framework, at least for the time being. We can revisit this but let's not stretch this PR more.

What's left:

  • Rebase the latest changes related to context and config files
  • Finalise the tables usage
  • Ensure a separate context is used for all CLI commands that batch GRPC calls
  • Ensure and update docs to reflect the latest state
  • Context.Project is UUID, not name. Either implement or fix that in the docs. (Added it in the description, the switch from UUID to name would be a separate PR)

Fixes: #1758
Fixes: #1754
Fixes: #1751
Fixes: #1749
Fixes: #1748

@rdimitrov rdimitrov force-pushed the improve-cli branch 6 times, most recently from 2a0b6d7 to dbb5202 Compare December 14, 2023 12:17
@rdimitrov rdimitrov changed the title WIP: Add common table implementation for cli and refactor cli use of flags WIP: Minder CLI improvements - table interface, refactor use of flags, context, etc. Dec 14, 2023
@rdimitrov rdimitrov force-pushed the improve-cli branch 2 times, most recently from 814fc18 to 486c863 Compare December 18, 2023 22:50
@rdimitrov rdimitrov requested a review from a team as a code owner December 18, 2023 22:50
@rdimitrov rdimitrov force-pushed the improve-cli branch 4 times, most recently from 1c4753a to 0e27c21 Compare December 19, 2023 13:23
@rdimitrov rdimitrov changed the title WIP: Minder CLI improvements - table interface, refactor use of flags, context, etc. Minder CLI improvements - table interface, refactor use of flags, context, etc. Dec 19, 2023
Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

This mostly looks good, the only thing I don't agree with is the app.IsProviderSupported function, which I think should be removed as it'll prevent us from adding other providers in the future. Let's not add code we have to remove later for little benefit.

JAORMX
JAORMX previously approved these changes Dec 19, 2023
Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

Trying this locally, the tables look good! the only one that looks a little off is the whoami list which looks a bit too slim. Is there a way for us to enforce minimal width?

+---------------+--------------------------------------+
|      KEY      |                VALUE                 |
+---------------+--------------------------------------+
| Minder Server | api.stacklok.com:443                 |
+---------------+--------------------------------------+
| Project       | jaormx /                             |
|               | xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx |
+---------------+--------------------------------------+

JAORMX
JAORMX previously approved these changes Dec 19, 2023
@JAORMX
Copy link
Contributor

JAORMX commented Dec 19, 2023

@rdimitrov what changed?

@rdimitrov
Copy link
Member Author

@rdimitrov what changed?

@JAORMX - Jakub merged his PR so I had to rebase. Also there was a diff in the make gen output.

@rdimitrov
Copy link
Member Author

Trying this locally, the tables look good! the only one that looks a little off is the whoami list which looks a bit too slim. Is there a way for us to enforce minimal width?

+---------------+--------------------------------------+
|      KEY      |                VALUE                 |
+---------------+--------------------------------------+
| Minder Server | api.stacklok.com:443                 |
+---------------+--------------------------------------+
| Project       | jaormx /                             |
|               | xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx |
+---------------+--------------------------------------+

Added a minimum width for tables of type key/value 👍

@rdimitrov rdimitrov requested a review from JAORMX December 19, 2023 14:38
Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

It still looks a little too slim but we can fix that later

@rdimitrov rdimitrov merged commit 4a4c381 into mindersec:main Dec 19, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment