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

log/logr/v2 with logr, zapr deps updated to v1.2.2 #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joelrebel
Copy link

@joelrebel joelrebel commented Jan 25, 2022

resolves #50

Note: This is a breaking change, hence versioned under /v2
to be imported as github.com/packethost/pkg/log/logr/v2

The current version of "github.com/packethost/pkg/log/logr" returns *PacketLogr as a logr.Logger,
which isn't possible anymore since in logr v1.2.2 logr.Logger is of type struct and it does not provide
the methods that old logr.Logger interface would declare.

https://github.com/go-logr/logr/blob/v1.2.2/logr.go#L230
https://github.com/go-logr/logr/blob/v0.4.0/logr.go#L148

With change, the NewPacketLogr() returns type *PacketLogr
which embeds the logr.Logger struct type, it inherits various methods
of the logr.Logger, although methods expecting a logr.Logger would need to
reference the *PacketLogr.Logger struct field.

The go-logr/logr v0.4.0 was an BETA API and is worth moving away from,
https://github.com/go-logr/logr/blob/v0.4.0/logr.go#L20

Note: This is a breaking change, hence versioned under /v2
      to be imported as "github.com/packethost/pkg/log/logr/v2"

With this change, returning *PacketLogr as a logr.Logger isn't
possible, since logr.Logger is of type struct and it does not provide
the methods that old logr.Logger interface would declare.

https://github.com/go-logr/logr/blob/v1.2.2/logr.go#L230
https://github.com/go-logr/logr/blob/v0.4.0/logr.go#L148

The NewPacketLogr() is updated to return type *PacketLogr
which embeds the logr.Logger struct type, it inherits various methods
of the logr.Logger, although methods expecting a logr.Logger would need to
reference the *PacketLogr.Logger struct field.

The go-logr/logr v0.4.0 was an BETA API and is worth moving away from,
https://github.com/go-logr/logr/blob/v0.4.0/logr.go#L20
@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #51 (b6efe11) into master (3874146) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #51   +/-   ##
=======================================
  Coverage   74.56%   74.56%           
=======================================
  Files           6        6           
  Lines         346      346           
=======================================
  Hits          258      258           
  Misses         70       70           
  Partials       18       18           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3874146...b6efe11. Read the comment docs.

joelrebel added a commit to tinkerbell/pbnj that referenced this pull request Jan 25, 2022
github.com/go-logr/logr v0.4.0 -> v1.2.2
github.com/go-logr/zapr v0.4.0 -> v1.2.2
github.com/bmc-toolbox/bmclib v0.4.14 -> v0.4.16
packethost/pkg/log/logr -> packethost/pkg/log/logr/v2

bmclib updated in this change becuse of the indirect (bmclib v0.4.14) dependency on the on logr v0.4.0.

expects PR merge: packethost/pkg#51
joelrebel added a commit to tinkerbell/pbnj that referenced this pull request Jan 25, 2022
github.com/go-logr/logr v0.4.0 -> v1.2.2
github.com/go-logr/zapr v0.4.0 -> v1.2.2
github.com/bmc-toolbox/bmclib v0.4.14 -> v0.4.16
packethost/pkg/log/logr -> packethost/pkg/log/logr/v2

bmclib updated in this change becuse of the indirect (bmclib v0.4.14) dependency on the on logr v0.4.0.

expects PR merge: packethost/pkg#51
@jacobweinstock
Copy link
Contributor

Hey @joelrebel, we aren't really using semantic versioning in this module. we're still at v0.0.0. And with that being that case do we really need to move to v2? moving to v2 feels like we would be taking on the tradeoffs of backward compatibility, support, and maintainability of a stable version. Also, this logr package, in my opinion, should be deprecated. The only consumer I'm aware of is PBnJ. And PBnJ should just use go-logr directly.

@joelrebel
Copy link
Author

joelrebel commented Jan 26, 2022

@jacobweinstock, The reasoning for v2 is keep the current code around, while having the newer version with breaking changes available under /v2 - the version number picked was more of a Go practice than anything else.

Deprecating this package would be ideal for lesser indirections and since the multi module nature of this repository makes it harder to maintain. The main motivation for this is tinkerbell/pbnj#114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Upgrade go-logr across the 1.0 breaking change
2 participants