Skip to content
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

feat: add flag to disable namespace warning #1017

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MichaelHindley
Copy link

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:

  • Code compiles correctly
  • Created/updated tests N/A ?
  • Extended the documentation N/A?

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.
@MichaelHindley MichaelHindley requested review from a team as code owners July 4, 2022 18:28
@hueifeng
Copy link
Member

hueifeng commented Jul 6, 2022

@MichaelHindley Please fix DCO

@MichaelHindley
Copy link
Author

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

@mukundansundar
Copy link
Collaborator

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

@pravinpushkar
Copy link
Contributor

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 ?

@mukundansundar
Copy link
Collaborator

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 ...

@pravinpushkar
Copy link
Contributor

@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
Copy link
Collaborator

@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
Copy link

codecov bot commented Aug 9, 2022

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.

@dapr-bot
Copy link
Collaborator

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
Copy link
Collaborator

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 dapr-bot added the stale label Oct 23, 2022
@dapr-bot
Copy link
Collaborator

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!

@mukundansundar
Copy link
Collaborator

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

@dapr-bot
Copy link
Collaborator

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
Copy link
Collaborator

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 dapr-bot added the stale label Feb 26, 2023
@yaron2
Copy link
Member

yaron2 commented Oct 3, 2023

@MichaelHindley please resolve the conflicts so we can merge.

@yaron2
Copy link
Member

yaron2 commented Jan 10, 2024

ping @MichaelHindley

@cicoyle
Copy link
Contributor

cicoyle commented Jul 26, 2024

ping @MichaelHindley - I think this would be a great value add for next release if you could please fix the merge conflicts?

@cicoyle cicoyle added this to the v1.15 milestone Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Commands print non-json warning when output set to json
8 participants