cli icon indicating copy to clipboard operation
cli copied to clipboard

feat: add flag to disable namespace warning

Open MichaelHindley opened this issue 2 years ago • 16 comments

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 avatar Jul 04 '22 18:07 MichaelHindley

@MichaelHindley Please fix DCO

hueifeng avatar Jul 06 '22 14:07 hueifeng

@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?

MichaelHindley avatar Jul 07 '22 06:07 MichaelHindley

@yaron2 Any issues with having a --silent or --quite flag to not print any warnings ?

mukundansundar avatar Jul 08 '22 05:07 mukundansundar

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 ?

pravinpushkar avatar Jul 18 '22 05:07 pravinpushkar

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 avatar Jul 18 '22 08:07 mukundansundar

@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 ?

pravinpushkar avatar Jul 18 '22 13:07 pravinpushkar

@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

mukundansundar avatar Jul 20 '22 06:07 mukundansundar

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.

codecov[bot] avatar Aug 09 '22 16:08 codecov[bot]

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!

dapr-bot avatar Sep 18 '22 14:09 dapr-bot

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!

dapr-bot avatar Oct 23 '22 01:10 dapr-bot

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!

dapr-bot avatar Oct 30 '22 01:10 dapr-bot

@MichaelHindley Please fix conflicts and changed implementation to what was discussed in #1015 is there any ETA on this PR?

mukundansundar avatar Dec 28 '22 09:12 mukundansundar

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!

dapr-bot avatar Jan 27 '23 09:01 dapr-bot

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!

dapr-bot avatar Feb 26 '23 11:02 dapr-bot

@MichaelHindley please resolve the conflicts so we can merge.

yaron2 avatar Oct 03 '23 20:10 yaron2

ping @MichaelHindley

yaron2 avatar Jan 10 '24 19:01 yaron2