Use zap sugared logger

We used zap from controller runtime, returning logr.Logger. Replace it with zap SugaredLoger which is much nicer to use, supporting both structured and formatted logging and named log levels. We use the default development configuration which require no special setup. Replace the logs using string concatenation or fmt.Sprintf() with log.Infof() or log.Errorf(), using quoted arguments for more clear logs. Signed-off-by: Nir Soffer <[email protected]>
RamenDR · Dec 4, 2024 · 927ebbb · 927ebbb
1 parent f730837
commit 927ebbb
Show file tree

Hide file tree

Showing 15 changed files with 56 additions and 63 deletions.
diff --git a/e2e/deployers/crud.go b/e2e/deployers/crud.go
@@ -74,7 +74,7 @@ func DeleteManagedClusterSetBinding(ctx types.Context, name, namespace string) e
 			return err
 		}
 
-		log.Info("ManagedClusterSetBinding " + name + " not found")
+		log.Infof("ManagedClusterSetBinding %q not found", name)
 	}
 
 	return nil
@@ -243,7 +243,7 @@ func CreatePlacementDecisionConfigMap(ctx types.Context, cmName string, cmNamesp
 			return fmt.Errorf("could not create configMap %q", cmName)
 		}
 
-		log.Info("ConfigMap " + cmName + " already Exists")
+		log.Infof("ConfigMap %q already Exists", cmName)
 	}
 
 	return nil
@@ -263,7 +263,7 @@ func DeleteConfigMap(ctx types.Context, cmName string, cmNamespace string) error
 			return fmt.Errorf("could not delete configMap %q", cmName)
 		}
 
-		log.Info("ConfigMap " + cmName + " not found")
+		log.Infof("ConfigMap %q not found", cmName)
 	}
 
 	return nil

diff --git a/e2e/deployers/discoveredapps.go b/e2e/deployers/discoveredapps.go
@@ -83,27 +83,27 @@ func (d DiscoveredApps) Undeploy(ctx types.Context) error {
 		return err
 	}
 
-	log.Info("Deleting discovered apps on " + drpolicy.Spec.DRClusters[0])
+	log.Infof("Deleting discovered apps on cluster %q", drpolicy.Spec.DRClusters[0])
 
 	// delete app on both clusters
 	if err := DeleteDiscoveredApps(ctx, namespace, drpolicy.Spec.DRClusters[0]); err != nil {
 		return err
 	}
 
-	log.Info("Deletting discovered apps on " + drpolicy.Spec.DRClusters[1])
+	log.Infof("Deletting discovered apps on cluster %q", drpolicy.Spec.DRClusters[1])
 
 	if err := DeleteDiscoveredApps(ctx, namespace, drpolicy.Spec.DRClusters[1]); err != nil {
 		return err
 	}
 
-	log.Info("Deleting namespace " + namespace + " on " + drpolicy.Spec.DRClusters[0])
+	log.Infof("Deleting namespace %q on cluster %q", namespace, drpolicy.Spec.DRClusters[0])
 
 	// delete namespace on both clusters
 	if err := util.DeleteNamespace(util.Ctx.C1.CtrlClient, namespace, log); err != nil {
 		return err
 	}
 
-	log.Info("Deleting namespace " + namespace + " on " + drpolicy.Spec.DRClusters[1])
+	log.Infof("Deleting namespace %q on cluster %q", namespace, drpolicy.Spec.DRClusters[1])
 
 	if err := util.DeleteNamespace(util.Ctx.C2.CtrlClient, namespace, log); err != nil {
 		return err

diff --git a/e2e/deployers/retry.go b/e2e/deployers/retry.go
@@ -25,7 +25,7 @@ func waitSubscriptionPhase(ctx types.Context, namespace, name string, phase subs
 
 		currentPhase := sub.Status.Phase
 		if currentPhase == phase {
-			log.Info(fmt.Sprintf("Subscription phase is %s", phase))
+			log.Infof("Subscription phase is %s", phase)
 
 			return nil
 		}

diff --git a/e2e/dractions/actions.go b/e2e/dractions/actions.go
@@ -52,7 +52,7 @@ func EnableProtection(ctx types.Context) error {
 	}
 
 	clusterName := placementDecision.Status.Decisions[0].ClusterName
-	log.Info("Workload running on " + clusterName)
+	log.Infof("Workload running on cluster %q", clusterName)
 
 	log.Info("Annotating placement")
 

diff --git a/e2e/dractions/actionsdiscoveredapps.go b/e2e/dractions/actionsdiscoveredapps.go
@@ -136,7 +136,7 @@ func failoverRelocateDiscoveredApps(ctx types.Context, action ramen.DRAction) er
 	}
 
 	// delete pvc and deployment from dr cluster
-	log.Info("Cleaning up discovered apps from " + currentCluster)
+	log.Infof("Cleaning up discovered apps from cluster %q", currentCluster)
 
 	if err = deployers.DeleteDiscoveredApps(ctx, namespaceInDrCluster, currentCluster); err != nil {
 		return err

diff --git a/e2e/dractions/retry.go b/e2e/dractions/retry.go
@@ -114,7 +114,7 @@ func waitDRPCPhase(ctx types.Context, client client.Client, namespace, name stri
 
 		currentPhase := drpc.Status.Phase
 		if currentPhase == phase {
-			log.Info(fmt.Sprintf("drpc phase is %s", phase))
+			log.Infof("drpc phase is %q", phase)
 
 			return nil
 		}
@@ -184,7 +184,7 @@ func waitDRPCDeleted(ctx types.Context, client client.Client, namespace string,
 				return nil
 			}
 
-			log.Info(fmt.Sprintf("failed to get drpc: %v", err))
+			log.Infof("Failed to get drpc: %s", err)
 		}
 
 		if time.Since(startTime) > util.Timeout {
@@ -213,7 +213,7 @@ func waitDRPCProgression(
 
 		currentProgression := drpc.Status.Progression
 		if currentProgression == progression {
-			log.Info(fmt.Sprintf("drpc progression is %s", progression))
+			log.Infof("drpc progression is %q", progression)
 
 			return nil
 		}

diff --git a/e2e/go.mod b/e2e/go.mod
@@ -5,7 +5,6 @@ go 1.22.5
 toolchain go1.22.7
 
 require (
-	github.com/go-logr/logr v1.4.2
 	github.com/ramendr/ramen/api v0.0.0-00010101000000-000000000000
 	github.com/spf13/viper v1.18.2
 	go.uber.org/zap v1.27.0
@@ -27,7 +26,7 @@ require (
 	github.com/fsnotify/fsnotify v1.7.0 // indirect
 	github.com/fxamacker/cbor/v2 v2.7.0 // indirect
 	github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32 // indirect
-	github.com/go-logr/zapr v1.3.0 // indirect
+	github.com/go-logr/logr v1.4.2 // indirect
 	github.com/go-openapi/jsonpointer v0.19.6 // indirect
 	github.com/go-openapi/jsonreference v0.20.2 // indirect
 	github.com/go-openapi/swag v0.22.4 // indirect

diff --git a/e2e/main_test.go b/e2e/main_test.go
@@ -10,9 +10,7 @@ import (
 
 	"github.com/ramendr/ramen/e2e/test"
 	"github.com/ramendr/ramen/e2e/util"
-	uberzap "go.uber.org/zap"
-	"go.uber.org/zap/zapcore"
-	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+	"go.uber.org/zap"
 )
 
 func init() {
@@ -24,23 +22,20 @@ func TestMain(m *testing.M) {
 
 	flag.Parse()
 
-	log := zap.New(zap.UseFlagOptions(&zap.Options{
-		Development: true,
-		ZapOpts: []uberzap.Option{
-			uberzap.AddCaller(),
-		},
-		TimeEncoder: zapcore.ISO8601TimeEncoder,
-	}))
+	logger, err := zap.NewDevelopment()
+	if err != nil {
+		panic(err)
+	}
+
+	log := logger.Sugar()
 
 	util.Ctx, err = util.NewContext(log, util.ConfigFile)
 	if err != nil {
-		log.Error(err, "unable to create new testing context")
-
-		panic(err)
+		log.Fatalf("Failed to create testing context: %s", err)
 	}
 
-	log.Info("Global setting", "Timeout", util.Timeout)
-	log.Info("Global setting", "Retry Interval", util.RetryInterval)
+	log.Infof("Using Timeout: %v", util.Timeout)
+	log.Infof("Using RetryInterval: %v", util.RetryInterval)
 
 	os.Exit(m.Run())
 }

diff --git a/e2e/test/context.go b/e2e/test/context.go
@@ -9,26 +9,26 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/go-logr/logr"
 	"github.com/ramendr/ramen/e2e/dractions"
 	"github.com/ramendr/ramen/e2e/types"
+	"go.uber.org/zap"
 )
 
 type Context struct {
 	workload types.Workload
 	deployer types.Deployer
 	name     string
-	logger   logr.Logger
+	logger   *zap.SugaredLogger
 }
 
-func NewContext(w types.Workload, d types.Deployer, log logr.Logger) Context {
+func NewContext(w types.Workload, d types.Deployer, log *zap.SugaredLogger) Context {
 	name := strings.ToLower(d.GetName() + "-" + w.GetName() + "-" + w.GetAppName())
 
 	return Context{
 		workload: w,
 		deployer: d,
 		name:     name,
-		logger:   log.WithName(name),
+		logger:   log.Named(name),
 	}
 }
 
@@ -52,7 +52,7 @@ func (c *Context) Namespace() string {
 	return c.name
 }
 
-func (c *Context) Logger() logr.Logger {
+func (c *Context) Logger() *zap.SugaredLogger {
 	return c.logger
 }
 

diff --git a/e2e/test/testing.go b/e2e/test/testing.go
@@ -4,21 +4,20 @@
 package test
 
 import (
-	"fmt"
 	"testing"
 
-	"github.com/go-logr/logr"
+	"go.uber.org/zap"
 )
 
 // T extends testing.T to use a custom logger.
 type T struct {
 	*testing.T
-	log logr.Logger
+	log *zap.SugaredLogger
 }
 
 // WithLog returns a t wrapped with a specified log.
 // nolint: thelper
-func WithLog(t *testing.T, log logr.Logger) *T {
+func WithLog(t *testing.T, log *zap.SugaredLogger) *T {
 	return &T{T: t, log: log}
 }
 
@@ -29,30 +28,30 @@ func (t *T) Log(msg string) {
 
 // Log writes a formatted message to the log.
 func (t *T) Logf(format string, args ...any) {
-	t.log.Info(fmt.Sprintf(format, args...))
+	t.log.Infof(format, args...)
 }
 
 // Error writes an error message to the log and mark the test as failed.
 func (t *T) Error(msg string) {
-	t.log.Error(nil, msg)
+	t.log.Error(msg)
 	t.T.Fail()
 }
 
 // Errorf writes a formatted error message to the log and markd the test as failed.
 func (t *T) Errorf(format string, args ...any) {
-	t.log.Error(nil, fmt.Sprintf(format, args...))
+	t.log.Errorf(format, args...)
 	t.T.Fail()
 }
 
 // Fatal writes an error message to the log and fail the text immediately.
 func (t *T) Fatal(msg string) {
-	t.log.Error(nil, msg)
+	t.log.Error(msg)
 	t.T.FailNow()
 }
 
 // Fatalf writes a formatted error message to the log and fail the text immediately.
 func (t *T) Fatalf(format string, args ...any) {
-	t.log.Error(nil, fmt.Sprintf(format, args...))
+	t.log.Errorf(format, args...)
 	t.T.FailNow()
 }
 
@@ -64,6 +63,6 @@ func (t *T) Skip(msg string) {
 
 // Skipf is equivalent to Logf followed by SkipNow.
 func (t *T) Skipf(format string, args ...any) {
-	t.log.Info(fmt.Sprintf(format, args...))
+	t.log.Infof(format, args...)
 	t.T.SkipNow()
 }
diff --git a/e2e/types/types.go b/e2e/types/types.go
@@ -4,7 +4,7 @@
 package types
 
 import (
-	"github.com/go-logr/logr"
+	"go.uber.org/zap"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -38,5 +38,5 @@ type Context interface {
 	Workload() Workload
 	Name() string
 	Namespace() string
-	Logger() logr.Logger
+	Logger() *zap.SugaredLogger
 }
diff --git a/e2e/util/config.go b/e2e/util/config.go
@@ -6,8 +6,8 @@ package util
 import (
 	"fmt"
 
-	"github.com/go-logr/logr"
 	"github.com/spf13/viper"
+	"go.uber.org/zap"
 )
 
 type PVCSpec struct {
@@ -27,7 +27,7 @@ type TestConfig struct {
 var config = &TestConfig{}
 
 //nolint:cyclop
-func ReadConfig(log logr.Logger, configFile string) error {
+func ReadConfig(log *zap.SugaredLogger, configFile string) error {
 	viper.SetDefault("ChannelName", defaultChannelName)
 	viper.SetDefault("ChannelNamespace", defaultChannelNamespace)
 	viper.SetDefault("GitURL", defaultGitURL)

diff --git a/e2e/util/context.go b/e2e/util/context.go
@@ -7,7 +7,7 @@ import (
 	"fmt"
 	"path/filepath"
 
-	"github.com/go-logr/logr"
+	"go.uber.org/zap"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/clientcmd"
@@ -33,7 +33,7 @@ type Cluster struct {
 }
 
 type Context struct {
-	Log logr.Logger
+	Log *zap.SugaredLogger
 	Hub Cluster
 	C1  Cluster
 	C2  Cluster
@@ -99,7 +99,7 @@ func setupClient(kubeconfigPath string) (*kubernetes.Clientset, client.Client, e
 	return k8sClientSet, ctrlClient, nil
 }
 
-func NewContext(log logr.Logger, configFile string) (*Context, error) {
+func NewContext(log *zap.SugaredLogger, configFile string) (*Context, error) {
 	var err error
 
 	ctx := new(Context)