-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
cli: Output message on success writing config entry #7806
Conversation
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 went hunting through the cli to see if there was prior art here.
consul services register service.hcl
Registered service: service-name
consul intention create foo bar
Created: foo => bar (allow)
consul intention match -source foo
foo => bar (allow)
There's not much but it looks like we do have a couple patterns:
- We use
<action>: <resource identifier>
, e.g.Config entry written: <identifier>
- We don't use "successfully"
- We don't use quotes in our identifier, e.g.
<name>/<kind>
- Our docs use
Configuration
but the cli help uses justconfig
so I think we're okay using config.
So to follow that pattern I think something more like:
Config entry written: global (proxy-defaults)
Also, can we add output to the delete as well?
👍 Good call out, I will take some time and review the CLI output of all our commands to make sure we are consistent here. |
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
Maybe add backport 1.8.x label? |
This provides a user with a better experience, knowing that the command worked appropriately. The output of the write/delete CLI commands are not going to be used in a bash script, in fact previously a success provided no ouput, so we do not have to worry about spurious text being injected into bash pipelines.
330aa89
to
503519e
Compare
Finally made a few changes to the output based on previous comments, re-requesting review. |
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.
New format looks good!
🍒✅ Cherry pick of commit ea683eb onto |
This provides a user with a better experience, knowing that the command worked appropriately. The output of the write/delete CLI commands are not going to be used in a bash script, in fact previously a success provided no ouput, so we do not have to worry about spurious text being injected into bash pipelines.
This helps users feel like an action occurred, rather than no output leading them to wonder if it worked or not.
This also converts the formatting output of config entries from
"proxy-defaults" / "global"
toproxy-defaults/global
as a slightly more readable form.