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

Improve Config CLI #41203

Merged
merged 1 commit into from
Jul 2, 2024
Merged

Improve Config CLI #41203

merged 1 commit into from
Jul 2, 2024

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Jun 14, 2024

Fixes #41131

  • Properly add help flags
  • Moved required options to parameters
  • Add a remove command

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/cli Related to quarkus cli (not maven/gradle/etc.) area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins labels Jun 14, 2024

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Jun 14, 2024

Ah cool, I tried it the other day and it was a bit hard to use. I'll have a look soon.

@gsmet gsmet self-requested a review June 14, 2024 15:43
@radcortez
Copy link
Member Author

Should be easier now :)

@gsmet
Copy link
Member

gsmet commented Jun 27, 2024

I'm having a look at it now.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I played with it a bit and here is my feedback.
Not saying everything should be fixed before we merged this PR but I'd like to have your feedback on my comments.

  1. I think quarkus config should display the help rather than defaulting to set. For me this is too important to push a random default.
  2. I think the feedback should be consistent with the feedback we have when adding an extension for instance.

We have:

Screenshot from 2024-06-27 14-22-24

In general, we should try to be consistent on the feedback side.

  1. When typing quarkus config enc, I end up with an error instead of the help being displaying (this is the case in main but also in this branch). I created Quarkus CLI - Some expected errors lead to a stacktrace and they shouldn't #41513 because even if you can tweak it here, I think it's something that is not ideal in our Picocli extension.

  2. I'm not sure of why I have to pass the secret with --secret in quarkus config enc --secret=secret, for the rest you pass the parameters as is (or at least that's what I did and it worked).
    Also I have no idea what to do with the output:

Encrypted Secret: DBLLBaf7o_i3u8lmJBb0rKJT13xvBXoQYjVFWFCJACl_tGU
Encryption Key: S6i5aNG6ySALS6021eMKbg

We probably need to explain what to do with it?

@radcortez
Copy link
Member Author

  1. I think quarkus config should display the help rather than defaulting to set. For me this is too important to push a random default.

OK

  1. I think the feedback should be consistent with the feedback we have when adding an extension for instance.

In general, we should try to be consistent on the feedback side.

OK

  1. When typing quarkus config enc, I end up with an error instead of the help being displaying (this is the case in main but also in this branch). I created Picocli - Some expected errors lead to a stacktrace and they shouldn't #41513 because even if you can tweak it here, I think it's something that is not ideal in our Picocli extension.

Yes, this is an issue with our extension rather than the CLI commands. When I looked into other examples, we also got the same error (for instance, quarkus extension add without any parameter also ends up with a stacktrace). I then forgot to open an issue about it. Thanks for opening one. It seems this happens only with subcommands.

  1. I'm not sure of why I have to pass the secret with --secret in quarkus config enc --secret=secret, for the rest you pass the parameters as is (or at least that's what I did and it worked).

Strange. It shouldn't be that way anymore. The previous version did have a --secret option but I've changed that to be a parameter.

Also I have no idea what to do with the output:

Encrypted Secret: DBLLBaf7o_i3u8lmJBb0rKJT13xvBXoQYjVFWFCJACl_tGU
Encryption Key: S6i5aNG6ySALS6021eMKbg

We probably need to explain what to do with it?

OK

@radcortez
Copy link
Member Author

This is how it is looking now:

Screenshot 2024-06-28 at 12 47 32

@gsmet
Copy link
Member

gsmet commented Jun 28, 2024

Love it!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@gsmet gsmet merged commit ca73df4 into quarkusio:main Jul 2, 2024
20 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jul 2, 2024
@gsmet gsmet mentioned this pull request Jul 2, 2024
@michalvavrik
Copy link
Member

Some changes here (Moved required options to parameters: SECRET, NAME, VALUE) are not backwards compatible, but I can't find it documented in https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.13 and this PR does not have a breaking-change label. Can you improve this, please?

@radcortez
Copy link
Member Author

Hey @michalvavrik sorry for the inconvenience.

We made the changes to fix some of the raised issues, plus make it consistent with other Quarkus CLI commands. I'll add that information now.

@michalvavrik
Copy link
Member

Hey @michalvavrik sorry for the inconvenience.

We made the changes to fix some of the raised issues, plus make it consistent with other Quarkus CLI commands. I'll add that information now.

np, but good to have it documented, thanks

@radcortez
Copy link
Member Author

https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.13#quarkus-cli

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/cli Related to quarkus cli (not maven/gradle/etc.) area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins kind/bugfix release/breaking-change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI - Config command - missing help text and unexpedted error stacktraces
3 participants