-
Notifications
You must be signed in to change notification settings - Fork 218
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
Stderr threshold issue fix #221
Stderr threshold issue fix #221
Conversation
Welcome @pierluigilenoci! |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pierluigilenoci The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -43,7 +43,10 @@ | |||
// Logs are written to standard error instead of to files. | |||
// -alsologtostderr=false | |||
// Logs are written to standard error as well as to files. | |||
// -stderrthreshold=ERROR | |||
// -stderrthreshold=INFO |
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.
this might have to land in new release because it's a breaking change.
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.
Please check the change inside the code. This is to maintain the pre-existing behavior.
However, there are no problems to land in a release.
klog.go
Outdated
@@ -436,6 +440,7 @@ func InitFlags(flagset *flag.FlagSet) { | |||
flagset.BoolVar(&logging.oneOutput, "one_output", logging.oneOutput, "If true, only write logs to their native severity level (vs also writing to each lower severity level)") | |||
flagset.BoolVar(&logging.skipLogHeaders, "skip_log_headers", logging.skipLogHeaders, "If true, avoid headers when opening log files") | |||
flagset.Var(&logging.stderrThreshold, "stderrthreshold", "logs at or above this threshold go to stderr") | |||
flagset.Var(&logging.alsoToStderrThreshold, "alsotostderrthreshold", "logs at or above this threshold go to stderr as well as files") |
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 personally find the new flag confusing but i can see you are trying to fix an already confusing behavior with it.
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 see no other way to fix this without breaking the old installations.
cc @kubernetes/sig-instrumentation-approvers |
fc6e969
to
6f2fbaa
Compare
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
@pierluigilenoci: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Please add a release note that summarizes the problem and your solution. |
Your current PR commits and descriptions assume that a reviewer is familiar with the problem or has the time to re-read all the prior issues. Can you make this easier to review by following https://www.kubernetes.dev/docs/guide/pull-requests/#commit-message-guidelines? I think you can also squash all of your commits. |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
This is still an issue |
So do you intend to get the PR into a mergeable state as discussed in the issue? |
@pohly this is the plan. To be implemented in the near future. 🚀 |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
/remove-lifecycle rotten |
@pierluigilenoci: this has been pending a long time. I am going to close it now to clean up pending PRs. Please reopen when there is something that is ready for merging. /close |
@pohly: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
This is my proposal to address these issues: #54 and #21
History of this bug:
#31
#42
#50
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #212
Special notes for your reviewer:
I added a further flag to decouple the behaviors between
alsoToStderr
and the default level oftoStderr
.Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: