Skip to content

Updates logr, zapr, packet logr, bmclib dependencies #114

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

Closed
joelrebel wants to merge 2 commits into from
Closed

Updates logr, zapr, packet logr, bmclib dependencies #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a4ece9
Updates logr, zapr, packet logr, bmclib dependencies
joelrebel Jan 25, 2022
01e6a28
lint: purge unused log variable
joelrebel Jan 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"fmt"
"os"

"github.com/packethost/pkg/log/logr"
"github.com/packethost/pkg/log/logr/v2"
"github.com/spf13/cobra"
v1 "github.com/tinkerbell/pbnj/api/v1"
v1Client "github.com/tinkerbell/pbnj/client"
Expand Down
6 changes: 3 additions & 3 deletions cmd/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
grpc_validator "github.com/grpc-ecosystem/go-grpc-middleware/validator"
grpc_prometheus "github.com/grpc-ecosystem/go-grpc-prometheus"
"github.com/packethost/pkg/grpc/authz"
"github.com/packethost/pkg/log/logr"
"github.com/packethost/pkg/log/logr/v2"
"github.com/spf13/cobra"
grpcsvr "github.com/tinkerbell/pbnj/grpc"
"github.com/tinkerbell/pbnj/pkg/http"
Expand Down Expand Up @@ -97,9 +97,9 @@ var (
)

httpServer := http.NewServer(metricsAddr)
httpServer.WithLogger(logger)
httpServer.WithLogger(logger.Logger)

if err := grpcsvr.RunServer(ctx, zaplog.RegisterLogger(logger), grpcServer, port, httpServer, grpcsvr.WithBmcTimeout(bmcTimeout)); err != nil {
if err := grpcsvr.RunServer(ctx, zaplog.RegisterLogger(logger.Logger), grpcServer, port, httpServer, grpcsvr.WithBmcTimeout(bmcTimeout)); err != nil {
logger.Error(err, "error running server")
os.Exit(1)
}
Expand Down
12 changes: 7 additions & 5 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ module github.com/tinkerbell/pbnj
go 1.16

require (
github.com/bmc-toolbox/bmclib v0.4.14-0.20211027184927-2f9a20e0a479
github.com/bmc-toolbox/bmclib v0.4.16-0.20211230160158-5afdbf3b6a65
github.com/cristalhq/jwt/v3 v3.0.9
github.com/dgrijalva/jwt-go v3.2.0+incompatible
github.com/equinix-labs/otel-init-go v0.0.4
github.com/fatih/color v1.7.0
github.com/go-logr/logr v0.4.0
github.com/go-logr/zapr v0.4.0
github.com/go-logr/logr v1.2.2
github.com/go-logr/zapr v1.2.2
github.com/go-test/deep v1.0.7
github.com/golang/protobuf v1.5.2
github.com/google/go-cmp v0.5.6
Expand All @@ -21,7 +21,7 @@ require (
github.com/mwitkow/go-proto-validators v0.3.2
github.com/onsi/gomega v1.10.4
github.com/packethost/pkg/grpc/authz v0.0.0-20210106215246-8e2e62dc8f0c
github.com/packethost/pkg/log/logr v0.0.0-20210106215246-8e2e62dc8f0c
github.com/packethost/pkg/log/logr/v2 v2.0.0
github.com/philippgille/gokv v0.6.0
github.com/philippgille/gokv/freecache v0.6.0
github.com/pkg/errors v0.9.1
Expand All @@ -33,10 +33,12 @@ require (
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.26.1
go.opentelemetry.io/otel v1.1.0
go.opentelemetry.io/otel/trace v1.1.0
go.uber.org/zap v1.16.0
go.uber.org/zap v1.19.0
goa.design/goa v2.2.5+incompatible
golang.org/x/sync v0.0.0-20201207232520-09787c993a3a
google.golang.org/grpc v1.41.0
google.golang.org/protobuf v1.27.1
gopkg.in/yaml.v2 v2.4.0
)

replace github.com/packethost/pkg/log/logr/v2 v2.0.0 => github.com/joelrebel/pkg/log/logr/v2 v2.0.0
52 changes: 26 additions & 26 deletions go.sum
View file
Open in desktop

Large diffs are not rendered by default.

10 changes: 5 additions & 5 deletions grpc/oob/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"time"

"github.com/google/go-cmp/cmp"
"github.com/packethost/pkg/log/logr"
"github.com/packethost/pkg/log/logr/v2"
v1 "github.com/tinkerbell/pbnj/api/v1"
"github.com/tinkerbell/pbnj/pkg/repository"
)
Expand All @@ -22,12 +22,12 @@ func TestParseAuth(t *testing.T) {
"nil Direct Auth": {input: &v1.Authn{Authn: &v1.Authn_DirectAuthn{DirectAuthn: nil}}, want: &repository.Error{Code: v1.Code_value["UNAUTHENTICATED"], Message: "no auth found", Details: nil}},
"nil auth": {input: nil, want: &repository.Error{Code: v1.Code_value["UNAUTHENTICATED"], Message: "no auth found", Details: nil}},
}
l, _, _ := logr.NewPacketLogr()
packetLogr, _, _ := logr.NewPacketLogr()
sm := make(chan string)
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
a := Accessory{
Log: l,
Log: packetLogr.Logger,
StatusMessages: sm,
}

Expand Down Expand Up @@ -66,14 +66,14 @@ func TestSendStatusMessage(t *testing.T) {
"without chan receiver": {runChanReceiver: false, want: nil},
}

l, _, _ := logr.NewPacketLogr()
packetLogr, _, _ := logr.NewPacketLogr()
sm := make(chan string)
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
var msgs []string
done := make(chan bool, 1)
a := Accessory{
Log: l,
Log: packetLogr.Logger,
StatusMessages: sm,
}

Expand Down
12 changes: 5 additions & 7 deletions grpc/rpc/bmc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,18 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap"
"github.com/packethost/pkg/log/logr"
"github.com/packethost/pkg/log/logr/v2"
"github.com/philippgille/gokv"
"github.com/philippgille/gokv/freecache"
v1 "github.com/tinkerbell/pbnj/api/v1"
"github.com/tinkerbell/pbnj/grpc/persistence"
"github.com/tinkerbell/pbnj/grpc/taskrunner"
"github.com/tinkerbell/pbnj/pkg/logging"
"github.com/tinkerbell/pbnj/pkg/zaplog"
)

const tempIPMITool = "/tmp/ipmitool"

var (
log logging.Logger
ctx context.Context
taskRunner *taskrunner.Runner
bmcService BmcService
Expand All @@ -39,8 +37,8 @@ func TestMain(m *testing.M) {

func setup() {
ctx = context.Background()
l, zapLogger, _ := logr.NewPacketLogr()
log = zaplog.RegisterLogger(l)
packetLogr, zapLogger, _ := logr.NewPacketLogr()
logger := zaplog.RegisterLogger(packetLogr.Logger)
ctx = ctxzap.ToContext(ctx, zapLogger)
f := freecache.NewStore(freecache.DefaultOptions)
s := gokv.Store(f)
Expand All @@ -52,10 +50,10 @@ func setup() {
taskRunner = &taskrunner.Runner{
Repository: repo,
Ctx: ctx,
Log: log,
Log: logger,
}
bmcService = BmcService{
Log: log,
Log: logger,
TaskRunner: taskRunner,
UnimplementedBMCServer: v1.UnimplementedBMCServer{},
}
Expand Down
10 changes: 5 additions & 5 deletions grpc/rpc/machine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap"
"github.com/onsi/gomega"
"github.com/packethost/pkg/log/logr"
"github.com/packethost/pkg/log/logr/v2"
"github.com/philippgille/gokv"
"github.com/philippgille/gokv/freecache"
v1 "github.com/tinkerbell/pbnj/api/v1"
Expand Down Expand Up @@ -62,8 +62,8 @@ func TestDevice(t *testing.T) {

ctx := context.Background()

l, zapLogger, _ := logr.NewPacketLogr()
logger := zaplog.RegisterLogger(l)
packetLogr, zapLogger, _ := logr.NewPacketLogr()
logger := zaplog.RegisterLogger(packetLogr.Logger)
ctx = ctxzap.ToContext(ctx, zapLogger)
f := freecache.NewStore(freecache.DefaultOptions)
s := gokv.Store(f)
Expand Down Expand Up @@ -166,8 +166,8 @@ func TestPower(t *testing.T) {

ctx := context.Background()

l, zapLogger, _ := logr.NewPacketLogr()
logger := zaplog.RegisterLogger(l)
packetLogr, zapLogger, _ := logr.NewPacketLogr()
logger := zaplog.RegisterLogger(packetLogr.Logger)
ctx = ctxzap.ToContext(ctx, zapLogger)
f := freecache.NewStore(freecache.DefaultOptions)
s := gokv.Store(f)
Expand Down
11 changes: 5 additions & 6 deletions grpc/rpc/task_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (

"github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap"
"github.com/onsi/gomega"
"github.com/packethost/pkg/log/logr"
"github.com/packethost/pkg/log/logr/v2"
"github.com/philippgille/gokv"
"github.com/philippgille/gokv/freecache"
"github.com/rs/xid"
Expand All @@ -26,8 +26,8 @@ func TestTaskFound(t *testing.T) {
Message: "",
Details: nil,
}
l, zapLogger, _ := logr.NewPacketLogr()
logger := zaplog.RegisterLogger(l)
packetLogr, zapLogger, _ := logr.NewPacketLogr()
logger := zaplog.RegisterLogger(packetLogr.Logger)
ctx = ctxzap.ToContext(ctx, zapLogger)
f := freecache.NewStore(freecache.DefaultOptions)
s := gokv.Store(f)
Expand Down Expand Up @@ -82,9 +82,8 @@ func TestRecordNotFound(t *testing.T) {
g := gomega.NewGomegaWithT(t)

ctx := context.Background()

l, zapLogger, _ := logr.NewPacketLogr()
logger := zaplog.RegisterLogger(l)
packetLogr, zapLogger, _ := logr.NewPacketLogr()
logger := zaplog.RegisterLogger(packetLogr.Logger)
ctx = ctxzap.ToContext(ctx, zapLogger)
f := freecache.NewStore(freecache.DefaultOptions)
s := gokv.Store(f)
Expand Down
6 changes: 3 additions & 3 deletions grpc/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,18 +114,18 @@ func RunServer(ctx context.Context, log logging.Logger, grpcServer *grpc.Server,
signal.Notify(sigChan, os.Interrupt)
go func() {
for range sigChan {
log.Info("sig received, shutting down PBnJ")
log.Info(0, "sig received, shutting down PBnJ")
grpcServer.GracefulStop()
<-ctx.Done()
}
}()

go func() {
<-ctx.Done()
log.Info("ctx cancelled, shutting down PBnJ")
log.Info(0, "ctx cancelled, shutting down PBnJ")
grpcServer.GracefulStop()
}()

log.Info("starting PBnJ gRPC server")
log.Info(0, "starting PBnJ gRPC server")
Copy link
Copy Markdown
Member

@jacobweinstock jacobweinstock Jan 26, 2022

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.

Copy link
Copy Markdown
Collaborator Author

@joelrebel joelrebel Feb 1, 2022

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

Copy link
Copy Markdown
Member

@jacobweinstock jacobweinstock Feb 4, 2022

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.

return grpcServer.Serve(listen)
}
20 changes: 10 additions & 10 deletions grpc/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
"time"

"github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap"
"github.com/packethost/pkg/log/logr"
"github.com/packethost/pkg/log/logr/v2"
"github.com/philippgille/gokv"
"github.com/philippgille/gokv/freecache"
"github.com/tinkerbell/pbnj/grpc/persistence"
Expand All @@ -24,8 +24,8 @@ import (
func TestRunServer(t *testing.T) {
ctx := context.Background()
ctx, cancel := context.WithTimeout(ctx, 9*time.Second)
l, zapLogger, err := logr.NewPacketLogr()
log := zaplog.RegisterLogger(l)
logger, zapLogger, err := logr.NewPacketLogr()
log := zaplog.RegisterLogger(logger.Logger)
ctx = ctxzap.ToContext(ctx, zapLogger)
if err != nil {
t.Fatal(err)
Expand All @@ -43,7 +43,7 @@ func TestRunServer(t *testing.T) {

grpcServer := grpc.NewServer()
httpServer := http.NewServer(fmt.Sprintf(":%d", port+1))
httpServer.WithLogger(l)
httpServer.WithLogger(logger.Logger)

g := new(errgroup.Group)
g.Go(func() error {
Expand All @@ -61,8 +61,8 @@ func TestRunServerSignals(t *testing.T) {
ctx := context.Background()
ctx, cancel := context.WithTimeout(ctx, 3*time.Second)
defer cancel()
l, zapLogger, err := logr.NewPacketLogr()
log := zaplog.RegisterLogger(l)
logger, zapLogger, err := logr.NewPacketLogr()
log := zaplog.RegisterLogger(logger.Logger)
ctx = ctxzap.ToContext(ctx, zapLogger)
if err != nil {
t.Fatal(err)
Expand All @@ -74,7 +74,7 @@ func TestRunServerSignals(t *testing.T) {
port := rand.Intn(max-min+1) + min
grpcServer := grpc.NewServer()
httpServer := http.NewServer(fmt.Sprintf(":%d", port+1))
httpServer.WithLogger(l)
httpServer.WithLogger(logger.Logger)

g := new(errgroup.Group)
g.Go(func() error {
Expand Down Expand Up @@ -105,16 +105,16 @@ func TestRunServerPortInUse(t *testing.T) {
ctx := context.Background()
ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
defer cancel()
l, zapLogger, err := logr.NewPacketLogr()
log := zaplog.RegisterLogger(l)
logger, zapLogger, err := logr.NewPacketLogr()
log := zaplog.RegisterLogger(logger.Logger)
ctx = ctxzap.ToContext(ctx, zapLogger)
if err != nil {
t.Fatal(err)
}

grpcServer := grpc.NewServer()
httpServer := http.NewServer(fmt.Sprintf(":%d", port+1))
httpServer.WithLogger(l)
httpServer.WithLogger(logger.Logger)

err = RunServer(ctx, log, grpcServer, strconv.Itoa(port), httpServer)
if err.Error() != "listen tcp :40039: bind: address already in use" {
Expand Down
6 changes: 3 additions & 3 deletions grpc/taskrunner/taskrunner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (
"time"

"github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap"
"github.com/packethost/pkg/log/logr"
"github.com/packethost/pkg/log/logr/v2"
"github.com/philippgille/gokv"
"github.com/philippgille/gokv/freecache"
"github.com/rs/xid"
Expand All @@ -27,8 +27,8 @@ func TestRoundTrip(t *testing.T) {
s := gokv.Store(f)
defer s.Close()
repo := &persistence.GoKV{Store: s, Ctx: ctx}
l, zapLogger, _ := logr.NewPacketLogr()
logger := zaplog.RegisterLogger(l)
packetLogr, zapLogger, _ := logr.NewPacketLogr()
logger := zaplog.RegisterLogger(packetLogr.Logger)
ctx = ctxzap.ToContext(ctx, zapLogger)
runner := Runner{
Repository: repo,
Expand Down
2 changes: 1 addition & 1 deletion pkg/logging/logging.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ import (

// Logger represent common interface for logging function.
type Logger interface {
logr.Logger
logr.LogSink
GetContextLogger(ctx context.Context) logr.Logger
}
4 changes: 2 additions & 2 deletions pkg/zaplog/zap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (

// Logger is a wrapper around zap.SugaredLogger.
type Logger struct {
logr.Logger
logr.LogSink
}

// GetContextLogger get and return a logger from a ctx.
Expand All @@ -21,5 +21,5 @@ func (l Logger) GetContextLogger(ctx context.Context) logr.Logger {

// RegisterLogger returns a logr and a zap logger (needed for use in grpc interceptors).
func RegisterLogger(log logr.Logger) logging.Logger {
return Logger{log}
return Logger{log.GetSink()}
}
4 changes: 2 additions & 2 deletions test/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"strconv"
"time"

"github.com/packethost/pkg/log/logr"
"github.com/packethost/pkg/log/logr/v2"
"github.com/tinkerbell/pbnj/cmd"
"github.com/tinkerbell/pbnj/test/runner"
)
Expand Down Expand Up @@ -48,7 +48,7 @@ func main() {

fmt.Println(*logLevel)
logger, _, _ := logr.NewPacketLogr(logr.WithLogLevel(*logLevel))
runner.RunTests(logger, cfgData)
runner.RunTests(logger.Logger, cfgData)
}

// Returns an int >= min, < max.
Expand Down