-
Notifications
You must be signed in to change notification settings - Fork 51
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
fix: Keyring output #2784
fix: Keyring output #2784
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2784 +/- ##
===========================================
- Coverage 78.83% 78.75% -0.08%
===========================================
Files 315 315
Lines 23810 23835 +25
===========================================
+ Hits 18769 18770 +1
- Misses 3668 3682 +14
- Partials 1373 1383 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
var cmd = &cobra.Command{ | ||
Use: "generate", | ||
Short: "Generate private keys", | ||
Long: `Generate private keys. | ||
Randomly generate and store private keys in the keyring. | ||
By default peer and encryption keys will be generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though: I wonder if we need to be more clear every time we mention encryption keys as there are different types of them for different context: encryption-at-rest, doc encryption, some ACP-related and so on. They grow like mushrooms these days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is well worth being very clear here - this is a destructive operation (information is overwritten), and the keys are accessible to users via the export
command. I have a preference for us to list each key with it's keyring name, and a short description as to what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a readme section dedicated to this already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we link to it from the cli? It is very handy if the CLI is self-explanatory, I find it annoying when I have to search for CLI documentation instead of just using --help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've copied the key management section into the help for the base keyring command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Keenan :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I agree that documentation should be linked in user facing cli documentation(help
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Keenan :)
Relevant issue(s)
Resolves #2759
Resolves #2758
Resolves #2757
Description
This PR adds more logs to the keyring to improve the user experience.
Tasks
How has this been tested?
Specify the platform(s) on which this was tested: