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.

Sign up for GitHub

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

Jump to bottom

WIP: adapt -vmodule support to logr v1.3.0 #383

Closed
wants to merge 1 commit into from
Closed

WIP: adapt -vmodule support to logr v1.3.0 #383

wants to merge 1 commit into from

Conversation

pohly
Copy link

@pohly pohly commented Sep 3, 2023 •
edited
Loading

What this PR does / why we need it:

LogSink.Enabled is sensitive to stack unwinding. logr v1.2.0 gets this wrong by calling LogSink.Enabled at different depths for Logger.Info and Logger.Enabled, therefore this test fails with that version. v1.3.0 will fix this.

Which issue(s) this PR fixes:
Related-to: go-logr/logr#215

Special notes for your reviewer:

A new logr release is needed.

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 3, 2023
@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pohly
Once this PR has been reviewed and has the lgtm label, please assign thockin for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 3, 2023
@k8s-ci-robot
Copy link

This issue is currently awaiting triage.

If klog contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Sep 3, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 3, 2023
@pohly pohly mentioned this pull request Sep 3, 2023
Closed
@pohly pohly force-pushed the enabled-stackdepth-test branch from df895fb to 05a75d8 Compare September 4, 2023 18:37
@pohly pohly force-pushed the enabled-stackdepth-test branch from 05a75d8 to 37f5288 Compare September 13, 2023 12:06
@pohly pohly changed the title WIP: klogr test: check vmodule support WIP: adapt -vmodule support to logr v1.3.0 Sep 13, 2023
@pohly pohly force-pushed the enabled-stackdepth-test branch 4 times, most recently from 545a57f to f5610c8 Compare September 13, 2023 12:25
@pohly pohly mentioned this pull request Sep 14, 2023
Merged
@pohly
update to logr 1.3.0
cc856bb 
LogSink.Enabled is sensitive to stack unwinding. logr v1.2.0  got this wrong
by calling LogSink.Enabled at different depths for Logger.Info and
Logger.Enabled, therefore this test fails with that version.

logr v1.3.0 changes the behavior to the more sane "LogSink.Enabled needs to
skip one level, regardless where it is called". The Discard log sink
implementation changed such that a discard logger doesn't have a sink at all
anymore. Go < 1.18 is no longer supported by logr.
@pohly pohly force-pushed the enabled-stackdepth-test branch from f5610c8 to cc856bb Compare October 27, 2023 06:05
@pohly
Copy link
Author

pohly commented Oct 27, 2023

Let's merge together with slog support in #384.

@pohly pohly closed this Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dims dims Awaiting requested review from dims

@harshanarayana harshanarayana Awaiting requested review from harshanarayana

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pohly @k8s-ci-robot