-
Notifications
You must be signed in to change notification settings - Fork 38
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
Updates logr, zapr, packet logr, bmclib dependencies #114
Conversation
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
Codecov Report
@@ Coverage Diff @@
## main #114 +/- ##
==========================================
- Coverage 80.93% 80.50% -0.43%
==========================================
Files 10 10
Lines 472 472
==========================================
- Hits 382 380 -2
- Misses 77 79 +2
Partials 13 13
Continue to review full report at Codecov.
|
grpcServer.GracefulStop() | ||
}() | ||
|
||
log.Info("starting PBnJ gRPC server") | ||
log.Info(0, "starting PBnJ gRPC server") |
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.
Hey @joelrebel, thanks for tackling this. I'm concerned that this logging signature now doesn't follow the go-logr documentation log.V(0).Info
. I don't think this allows us to use go-logr
as is expected/intended. I think this is only necessary because of the way logging is being handled in general here in PBnJ. It's not very good 😞 (I wrote it!).
What are your thoughts around removing thepackethost/pkg/log/logr
dependency entirely and just using go-logr
? I apologize. I know it's not what you were wanting to do here and the work to move to go-logr
touches a lot. I'm willing to do the work to do this update if that's helpful.
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.
hey @jacobweinstock, apologies, I haven't noticed the reply earlier.
Yep! totally its worth moving to go-logr
and removing the dep packethost/pkg/log/logr
, let me know if I should close this PR
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.
Yeah, if you wouldn't mind closing this one. I opened #116 to do the move to go-logr
directly.
Hello PBNJ maintainers, we build and distribute an opinionated version of PBNJ as part of the EKS Anywhere artifact catalog. PBNJ has a go mod dependency on bmc-toolbox/bmclib, which had a dependency on the LGPLv3-licensed gebn/bmc. This was fixed in bmc-toolbox/bmclib#257, and the intention was to make our build of PBNJ use this newer commit in its Go mod through a patch. However, (as you all know) bmclib has since upgraded logging dependencies, so directly consuming this commit was causing incompatibility issues with some other Go mods such as packethost/pkg/log/logr which are using much older versions of In order to get around this, we follow the approach of using a local bmclib module version of bmclib which has the PR's commit cherry-picked. TLDRWould really love for this fix to be merged! (or moving to go-logr) 🎉 |
Hey @abhay-krishna. This new PR #116 should hopefully satisfy this. Mind checking it out? |
Thank you! I'll take a look asap! |
Description
logr, zapr, packetlogr, bmclib dependencies updated
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 because of the indirect (bmclib v0.4.14) dependency on the on logr v0.4.0.
note:
expects linked PR merged packethost/pkg#51 and tagged with
log/logr/v2.0.0
once the linked PR is approved, merged and tagged, go.mod is to be updated to remove the
replace
directive2a4ece9#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R44
Why is this needed
The current logr, zapr, packethost/pkg/logr depend on the older 'beta'
logr
package,this change updates the logging dependencies to the current latest.
for more information see packethost/pkg#51
How are existing users impacted? What migration steps/scripts do we need?
No impact on existing users identified