⚠️ Bump github.com/go-logr/zapr & github.com/go-logr/logr to v1.1.0 #1593

morlay · 2021-07-15T10:16:01Z

Bump github.com/go-logr/zapr & github.com/go-logr/logr to v1.0.0
with logr apis refactor

k8s-ci-robot · 2021-07-15T10:16:09Z

Welcome @morlay!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-07-15T10:16:09Z

Hi @morlay. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 · 2021-07-15T10:16:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: morlay
To complete the pull request process, please assign droot after the PR has been reviewed.
You can assign the PR to them by writing /assign @droot in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vincepri · 2021-07-15T15:23:39Z

/ok-to-test

vincepri · 2021-07-15T15:24:00Z

/milestone v0.10.x

vincepri · 2021-07-15T15:24:20Z

/retitle ⚠️ Bump github.com/go-logr/zapr & github.com/go-logr/logr to v1.0.0

joelanford · 2021-07-20T13:36:00Z

There might be some impact on this change with respect to the zap-log-level and zap-stacktrace-level flags because negative V values are now treated the same as 0.

The zap error and panic levels correspond to negative logr levels, so we need to either verify that those flags continue working as they always have, or if they don't, we need to tweak pkg/log/zap to compensate for this change.

For example, this test may fail with the change to logr/zapr v1.0.0:

controller-runtime/pkg/log/zap/zap_test.go

Lines 538 to 539 in 40cffdc

    
           logger.V(-3).Info("test -3") // Log a panic, since V(-1*N) for all N > 0 is not permitted. 
        
           Expect(logOut.String()).To(ContainSubstring(`"level":"dpanic"`))

morlay · 2021-08-11T02:01:41Z

Block by k8s.io/klog/v2, waiting for a new release
branch main already upgraded https://github.com/kubernetes/klog

joelanford · 2021-08-24T18:10:13Z

Moving this to a future milestone. The kubernetes/kubernetes project is still on logr 0.4.0 for the 1.22 release, so controller-runtime will stay on 0.4.0 for v0.10.x to stay aligned with upstream.

/milestone Next

rashedkvm · 2021-09-28T01:33:19Z

The absence of this merge blocking is upgrading some other components. 🙏

rashedkvm · 2021-10-12T01:45:30Z

Requesting to prioritize this issue. Thank you!!!

randomvariable · 2021-10-20T14:34:02Z

We should probably merge this in, but is there going to be some downstream impact for consumers, in which case this should go to v0.11?

vincepri · 2021-10-20T14:36:33Z

Yes to v0.11

- `k8s.io/*` bundle to `v0.22.2` - `github.com/golang/mock` to `v1.6.0` (is not compiled into final binary but checked by dependabot) - `github.com/miekg/dns` to `v1.1.43` The latest `sigs.k8s.io/external-dns` is problematic in terms it uses latest `github.com/go-logr/logr` which is incompatible with previous versions. This causes issues in `controller-runtime` and will require code changes on our side (another PR). [This PR](kubernetes-sigs/controller-runtime#1593 (comment)) would be unblock - github.com/go-logr/logr - sigs.k8s.io/external-dns

- `k8s.io/*` bundle to `v0.22.2` - `github.com/golang/mock` to `v1.6.0` (is not compiled into final binary but checked by dependabot) - `github.com/miekg/dns` to `v1.1.43` The latest `sigs.k8s.io/external-dns` is problematic in terms it uses latest `github.com/go-logr/logr` which is incompatible with previous versions. This causes issues in `controller-runtime` and will require code changes on our side (another PR). [This PR](kubernetes-sigs/controller-runtime#1593 (comment)) would be unblock - github.com/go-logr/logr - sigs.k8s.io/external-dns Signed-off-by: kuritka <kuritka@gmail.com>

- `k8s.io/*` bundle to `v0.22.2` (controller-gen tool makes changes in CRD description after bump) - `github.com/golang/mock` to `v1.6.0` (is not compiled into final binary but checked by dependabot) - `github.com/miekg/dns` to `v1.1.43` - `github.com/infobloxopen/infoblox-go-client` to `v1.1.1` (changes argument CreateTXTRecord from integer to uint) The latest `sigs.k8s.io/external-dns` is problematic in terms it uses latest `github.com/go-logr/logr` which is incompatible with previous versions. This causes issues in `controller-runtime` and will require code changes on our side (another PR). [This PR](kubernetes-sigs/controller-runtime#1593 (comment)) would be unblock - github.com/go-logr/logr - sigs.k8s.io/external-dns Signed-off-by: kuritka <kuritka@gmail.com>

related to #697 `k8s.io/*` bundle to `v0.22.2` (controller-gen tool makes changes in CRD description after bump) - `github.com/golang/mock` to `v1.6.0` (is not compiled into final binary but checked by dependabot) - `github.com/miekg/dns` to `v1.1.43` - `github.com/infobloxopen/infoblox-go-client` to `v1.1.1` (changes argument CreateTXTRecord from integer to uint) The latest `sigs.k8s.io/external-dns` is problematic in terms it uses latest `github.com/go-logr/logr` which is incompatible with previous versions. This causes issues in `controller-runtime` and will require code changes on our side (another PR). [This PR](kubernetes-sigs/controller-runtime#1593 (comment)) would be unblock - github.com/go-logr/logr - sigs.k8s.io/external-dns Signed-off-by: kuritka <kuritka@gmail.com>

related to #697 `k8s.io/*` bundle to `v0.22.2` (controller-gen tool makes changes in CRD description after bump) - `github.com/golang/mock` to `v1.6.0` (is not compiled into final binary but checked by dependabot) - `github.com/miekg/dns` to `v1.1.43` - `github.com/infobloxopen/infoblox-go-client` to `v1.1.1` (changes argument CreateTXTRecord from integer to uint) `sigs.k8s.io/controller-runtime` to `v0.10.2` The latest `sigs.k8s.io/external-dns` is problematic in terms it uses latest `github.com/go-logr/logr` which is incompatible with previous versions. This causes issues in `controller-runtime` and will require code changes on our side (another PR). [This PR](kubernetes-sigs/controller-runtime#1593 (comment)) would be unblock - github.com/go-logr/logr - sigs.k8s.io/external-dns Signed-off-by: kuritka <kuritka@gmail.com>

varshaprasad96 · 2021-10-29T22:17:45Z

If we are going to cut a release with this breaking change + k8s 1.22 deps, can we merge this before #1709 ?

joelanford

In the GoDoc for logr.Logger, I see:

Implementations of LogSink should provide their own constructors that return Logger, not LogSink.

It seems like we're violating this guidance with the DelegatingLogSink constructor.

I'm guessing the reason is that we somehow need to call Fulfill() eventually, but we can't if our concrete DelegatingLogger is lost behind the logr.Logger abstraction.

Is there a way to overcome this? Perhaps we can fudge the guidance and have a NewDelegatingLogger that returns a DelegatingLogger rather than a logr.Logger? And then callers could call Fulfill() but then also unwrap it to get at an underlying logr.Logger?

Maybe related, do we need DelegatingLogSink to be exported? I wonder if we can restore DelegatingLogger and unexport DelegatingLogSink.

I'm totally spitballing here. Not sure what cascading effects this would cause.

joelanford · 2021-11-01T20:53:06Z

pkg/log/zap/zap_test.go

@@ -535,9 +539,10 @@ var _ = Describe("Zap log level flag options setup", func() {
 				logOut.Truncate(0)
 				logger.V(4).Info("test 4") // Should not be logged
 				Expect(logOut.String()).To(BeEmpty())
-				logger.V(-3).Info("test -3") // Log a panic, since V(-1*N) for all N > 0 is not permitted.
-				Expect(logOut.String()).To(ContainSubstring(`"level":"dpanic"`))
+				logger.V(-3).Info("test -3") // Since logr v1.0.0, negative logr level will be force to 0 https://github.com/go-logr/logr/blob/master/logr.go#L269-L279、.


This is a runtime breaking change, so we need to make it very apparent in release notes what this means for users currently using negative V() values.

joelanford · 2021-11-01T20:56:44Z

pkg/log/zap/zap_test.go

-				logger.V(-3).Info("test -3") // Log a panic, since V(-1*N) for all N > 0 is not permitted.
-				Expect(logOut.String()).To(ContainSubstring(`"level":"dpanic"`))
+				logger.V(-3).Info("test -3") // Since logr v1.0.0, negative logr level will be force to 0 https://github.com/go-logr/logr/blob/master/logr.go#L269-L279、.
+				Expect(logOut.String()).To(ContainSubstring("test -3"))


Can we have the unit test try various negative numbers and have the expectations include a test for the level we expect them to come out as?

pkg/builder/controller_test.go

morlay · 2021-11-02T05:23:03Z

@joelanford

Have to return logr.Logger, it is an struct. if we return custom struct, will be not compatible with other pkgs.

So I have upgrade codes, and make DelegatingLogSink as private.

and added new NewDelegatingLogger(logSink logr.LogSink) logr.Logger

For FullFill could be use

Log.GetSink().(canFulfill).Fulfill(l.GetSink())

or added a new function to safe fill

func Fulfill(l *logr.Logger, logSink logr.LogSink) {
   if canFulfill, ok := l.GetSink().(interface{ Fulfill(logSink logr.LogSink) }); ok {
     canFulfill.Fulfill(logSink)
   }
}

k8s-ci-robot · 2021-11-02T05:25:16Z

@morlay: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-controller-runtime-apidiff-master	`2c829ae`	link	false	`/test pull-controller-runtime-apidiff-master`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-ci-robot · 2021-11-02T17:42:45Z

@morlay: 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.

morlay · 2021-11-04T02:46:42Z

Sames previous version already meged. Close this

see: - kubernetes-sigs/controller-runtime#1607 - kubernetes-sigs/controller-runtime#1593

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 15, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 15, 2021

k8s-ci-robot requested review from mengqiy and shawn-hurley July 15, 2021 10:16

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 15, 2021

morlay changed the title ~~🌱 Bump github.com/go-logr/zapr & github.com/go-logr/logr to v1.0.0~~ 🌱 Bump github.com/go-logr/zapr & github.com/go-logr/logr to v1.0.0 Jul 15, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 15, 2021

k8s-ci-robot added this to the v0.10.x milestone Jul 15, 2021

k8s-ci-robot changed the title ~~🌱 Bump github.com/go-logr/zapr & github.com/go-logr/logr to v1.0.0~~ ⚠️ Bump github.com/go-logr/zapr & github.com/go-logr/logr to v1.0.0 Jul 15, 2021

morlay force-pushed the master branch 4 times, most recently from 730ad94 to 3dfe5be Compare July 16, 2021 01:58

alvaroaleman mentioned this pull request Jul 28, 2021

controller-runtime is broken by github.com/go-logr/logr v1.0.0 #1607

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2021

morlay force-pushed the master branch from 3dfe5be to 5a530d1 Compare August 11, 2021 01:49

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2021

morlay marked this pull request as draft August 11, 2021 01:56

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 Aug 11, 2021

morlay mentioned this pull request Aug 11, 2021

logr has tagged v1.0.0-rc1 kubernetes/klog#241

Closed

morlay force-pushed the master branch from 8546fda to 61a18e3 Compare September 22, 2021 03:33

nixpanic mentioned this pull request Sep 28, 2021

rebase: bump k8s.io/klog/v2 from 2.10.0 to 2.20.0 ceph/ceph-csi#2525

Closed

ckotzbauer mentioned this pull request Oct 9, 2021

deps: update module sigs.k8s.io/controller-runtime to v0.10.3 ckotzbauer/access-manager#156

Closed

1 task

varshaprasad96 mentioned this pull request Oct 29, 2021

⚠️ bump k8s to 1.23-alpha.3 #1709

Merged

nixpanic mentioned this pull request Nov 1, 2021

rebase: bump k8s.io/klog/v2 from 2.10.0 to 2.30.0 ceph/ceph-csi#2590

Closed

joelanford reviewed Nov 1, 2021

View reviewed changes

morlay force-pushed the master branch 2 times, most recently from ad5bc6e to 3079894 Compare November 2, 2021 05:16

Bump github.com/go-logr/zapr & github.com/go-logr/logr to v1

2c829ae

morlay force-pushed the master branch from 3079894 to 2c829ae Compare November 2, 2021 05:23

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2021

morlay closed this Nov 4, 2021

thorsteinnth added a commit to spotinst/spotctl that referenced this pull request Dec 10, 2021

logr 1.2.1 -> 0.4.0

4e17e78

see: - kubernetes-sigs/controller-runtime#1607 - kubernetes-sigs/controller-runtime#1593

This was referenced Dec 14, 2021

Bump github.com/go-logr/logr from 0.4.0 to 1.2.2 rabbitmq/cluster-operator#920

Closed

Bump github.com/go-logr/logr from 0.4.0 to 1.2.2 rabbitmq/messaging-topology-operator#269

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Bump github.com/go-logr/zapr & github.com/go-logr/logr to v1.1.0 #1593

⚠️ Bump github.com/go-logr/zapr & github.com/go-logr/logr to v1.1.0 #1593

morlay commented Jul 15, 2021

k8s-ci-robot commented Jul 15, 2021

k8s-ci-robot commented Jul 15, 2021

k8s-ci-robot commented Jul 15, 2021

vincepri commented Jul 15, 2021

vincepri commented Jul 15, 2021

vincepri commented Jul 15, 2021

joelanford commented Jul 20, 2021

morlay commented Aug 11, 2021

joelanford commented Aug 24, 2021

rashedkvm commented Sep 28, 2021 •

edited

rashedkvm commented Oct 12, 2021

randomvariable commented Oct 20, 2021

vincepri commented Oct 20, 2021

varshaprasad96 commented Oct 29, 2021

joelanford left a comment

joelanford Nov 1, 2021

joelanford Nov 1, 2021

morlay commented Nov 2, 2021 •

edited

k8s-ci-robot commented Nov 2, 2021 •

edited

k8s-ci-robot commented Nov 2, 2021

morlay commented Nov 4, 2021 •

edited

⚠️ Bump github.com/go-logr/zapr & github.com/go-logr/logr to v1.1.0 #1593

⚠️ Bump github.com/go-logr/zapr & github.com/go-logr/logr to v1.1.0 #1593

Conversation

morlay commented Jul 15, 2021

k8s-ci-robot commented Jul 15, 2021

k8s-ci-robot commented Jul 15, 2021

k8s-ci-robot commented Jul 15, 2021

vincepri commented Jul 15, 2021

vincepri commented Jul 15, 2021

vincepri commented Jul 15, 2021

joelanford commented Jul 20, 2021

morlay commented Aug 11, 2021

joelanford commented Aug 24, 2021

rashedkvm commented Sep 28, 2021 • edited

rashedkvm commented Oct 12, 2021

randomvariable commented Oct 20, 2021

vincepri commented Oct 20, 2021

varshaprasad96 commented Oct 29, 2021

joelanford left a comment

Choose a reason for hiding this comment

joelanford Nov 1, 2021

Choose a reason for hiding this comment

joelanford Nov 1, 2021

Choose a reason for hiding this comment

morlay commented Nov 2, 2021 • edited

k8s-ci-robot commented Nov 2, 2021 • edited

k8s-ci-robot commented Nov 2, 2021

morlay commented Nov 4, 2021 • edited

rashedkvm commented Sep 28, 2021 •

edited

morlay commented Nov 2, 2021 •

edited

k8s-ci-robot commented Nov 2, 2021 •

edited

morlay commented Nov 4, 2021 •

edited