-
Notifications
You must be signed in to change notification settings - Fork 203
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
base: master
Are you sure you want to change the base?
feat: add flag to disable namespace warning #1017
Conversation
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 Please fix DCO |
@hueifeng I will when I start implementing the changes as discussed in #1015. |
@yaron2 Any issues with having a |
As suggested by @mukundansundar
and handle the console output in
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 ? |
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
@@ 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 |
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 |
ping @MichaelHindley - I think this would be a great value add for next release if you could please fix the merge conflicts? |
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: