-
Notifications
You must be signed in to change notification settings - Fork 110
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 the CLI experience #160
Improve the CLI experience #160
Conversation
Nice the CLI output you've shown here certainly looks more helpful for folks running the tools interactively.
As you've identified, this feels like it could break workflows. If someone is piping the output of the tool into a file, that file won't be parseable as JSON after this change. I wonder whether we add a What do you think @asraa @hosseinsia @mnm678 @trishankatdatadog ? |
My vote would be for the |
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 left a few nits, but mostly this looks good. My only concern relates to my comment above about root-keys
.
Signed-off-by: Radoslav Dimitrov <[email protected]>
Signed-off-by: Radoslav Dimitrov <[email protected]>
Signed-off-by: Radoslav Dimitrov <[email protected]>
Signed-off-by: Radoslav Dimitrov <[email protected]>
Signed-off-by: Radoslav Dimitrov <[email protected]>
Signed-off-by: Radoslav Dimitrov <[email protected]>
Signed-off-by: Radoslav Dimitrov <[email protected]>
Signed-off-by: Radoslav Dimitrov <[email protected]>
Signed-off-by: Radoslav Dimitrov <[email protected]>
Signed-off-by: Radoslav Dimitrov <[email protected]>
Signed-off-by: Radoslav Dimitrov <[email protected]>
Thanks for your feedback, @mnm678 👍 I'm leaning towards being verbose by default as it's more common for new users to use it interactively in the beginning (making it more user-friendly but also have the As for the other commands, I don't think they can break something, so I guess we can keep their messages verbose by default, right? |
Signed-off-by: Radoslav Dimitrov <[email protected]>
2b9f12f
to
ea8ff0c
Compare
Pull Request Test Coverage Report for Build 1319883009
💛 - Coveralls |
I don't think it's such a breaking change. We have done #143 which was definitely breaking, and we haven't seen complaints yet. But anyone who depends on this definitely should speak up. I'm in favour of more, not less, explicit error messages by default. New users should not just see something like |
I think so too 👍 In that sense, can we agree on leaving the more verbose output enabled by default and adding a |
Can we call it To address the concern about command output going directly into another command, we should put informative/diagnostic output on stderr and conventional output (JSON for other commands) on stdout. See https://pubs.opengroup.org/onlinepubs/9699919799/functions/stderr.html |
Neat 👍 Will do that. |
…stderr Signed-off-by: Radoslav Dimitrov <[email protected]>
I've applied the suggestions and added -q|--quiet mode for Although now that I'm thinking having both quiet mode and stream redirect might be unnecessary. Having the latter makes the quiet option obsolete I guess. |
Yes, sorry I was unclear in my original response. You are correct. We should not need quiet mode because if diagnostics go to stderr and conventional output goes to stdout then |
Exactly 👍 I'll revert the quiet mode then |
Not necessary because the informative msg is streamed to stderr ergo it won't break piping the output Signed-off-by: Radoslav Dimitrov <[email protected]>
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 Radoslav! Could we get some review from others, especially our more Go savvy maintainers?
This PR and various subsequent changes have made it considerably more noisy to use go-tuf as a library. Would it be possible to thread an |
The following PR adds informative messages about each CLI command upon successful completion. Closes #11
One thing I'd like to note is the output for
tuf root-keys
previously consisted only of the JSON encoded root keys and now there's a helpful message before that. This might break stuff in case there's some automation built around it, so let me know if it might make sense to revert that change specifically.Examples of the resulting output: