cli
cli copied to clipboard
feat: add flag to disable namespace warning
Description
Adds flag to disable printing warning messages for default namespaces.
print.WarningStatusEvents is also used to report on errors that occur during command execution.
First it was considered to check for the flag within print.WarningStatusEvent, but this does not seem in tune with the spirit of this flag, which is to prevent breaking output formats due to a warning that is always printed.
For the context of this flag, its thus explicitly named so its clear only the namespace warning is covered, and not all warnings that are generated throughout the CLI.
Names likes noWarning, disableWarning were also considered.
Single char shorthand flag was also considered.
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #1015
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
- [x] Code compiles correctly
- [ ] Created/updated tests N/A ?
- [ ] Extended the documentation N/A?
@MichaelHindley Please fix DCO
@hueifeng I will when I start implementing the changes as discussed in #1015.
@mukundansundar can I assume the changes from your comment in regard to --quiet/silent
are green to contribute?
@yaron2 Any issues with having a --silent
or --quite
flag to not print any warnings ?
As suggested by @mukundansundar Alternate approach can be having the flag at RootCmd level like, So that it is available for every command.
RootCmd.PersistentFlags().BoolVarP(&silent, "silent", "s", false, "Silence all output except errors")
and handle the console output in print.go
at success, info and warning level, something like -
func InfoStatusEvent(w io.Writer, fmtstr string, a ...interface{}) {
if silent {
return
}
......
Thoughts @mukundansundar @MichaelHindley ?
As suggested by @mukundansundar Alternate approach can be having the flag at RootCmd level like, So that it is available for every command.
RootCmd.PersistentFlags().BoolVarP(&silent, "silent", "s", false, "Silence all output except errors")
and handle the console output in
print.go
at success, info and warning level, something like -func InfoStatusEvent(w io.Writer, fmtstr string, a ...interface{}) { if silent { return } ......
Thoughts @mukundansundar @MichaelHindley ?
If we want to go for this flag, it needs to be applied retroactively to all the print statements that we do today ...
@mukundansundar yes, thats correct. In print.go file we will have to do handle this in 3 places- success, info and warning print functions. Is there any other simpler way ?
@mukundansundar yes, thats correct. In print.go file we will have to do handle this in 3 places- success, info and warning print functions. Is there any other simpler way ?
Can we clearly define as to what output is something that should be there regardless of the silent flag or not
For dapr init... dapr installed successfully alone and not any of the other output except errors if any...
Similarly for other commands that use the print.go functions for output all will be affected by the silent flag... So some clear definition of what should be affected vs what should not be will need to be thought through and appropriate changes be made.... For those conversations I suggest we do it in the parent issue and not in this PR
Codecov Report
Merging #1017 (05345a6) into master (d229506) will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## master #1017 +/- ##
=======================================
Coverage 29.34% 29.34%
=======================================
Files 35 35
Lines 2334 2334
=======================================
Hits 685 685
Misses 1574 1574
Partials 75 75
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
@MichaelHindley Please fix conflicts and changed implementation to what was discussed in #1015 is there any ETA on this PR?
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
@MichaelHindley please resolve the conflicts so we can merge.
ping @MichaelHindley