update logr to 1.2.4 and fix Stash client

In version 1.2.4 a [change](go-logr/logr#166) was introduced that significantly changed the assumptions about logr's API, i.e. that `logr.Logger{} != logr.Discard()`. This condition doesn't evaluate to true with 1.2.4, anymore, and the default value of the `logr.Logger` type is now usable as a replacement for `logr.Discard()`. Therefore we do not need to check if the value has been provided, anymore: If it is not provided, the default one is used which is effectively a discarding logger. closes #209 Signed-off-by: Max Jonas Werner <[email protected]>
fluxcd · Apr 19, 2023 · 4d35975 · 4d35975
1 parent 926eae0
commit 4d35975
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 21 deletions.
diff --git a/go.mod b/go.mod
@@ -6,7 +6,7 @@ require (
 	github.com/ProtonMail/go-crypto v0.0.0-20220714114130-e85cedf506cd
 	github.com/go-git/go-billy/v5 v5.3.1
 	github.com/go-git/go-git/v5 v5.4.2
-	github.com/go-logr/logr v1.2.3
+	github.com/go-logr/logr v1.2.4
 	github.com/go-logr/zapr v1.2.3
 	github.com/google/go-cmp v0.5.9
 	github.com/google/go-github/v49 v49.1.0
@@ -22,6 +22,7 @@ require (
 	golang.org/x/crypto v0.0.0-20220722155217-630584e8d5aa
 	golang.org/x/oauth2 v0.6.0
 	golang.org/x/time v0.3.0
+	k8s.io/utils v0.0.0-20230406110748-d93618cff8a2
 )
 
 // Fix CVE-2022-28948

diff --git a/go.sum b/go.sum
@@ -34,8 +34,8 @@ github.com/go-git/go-git-fixtures/v4 v4.2.1/go.mod h1:K8zd3kDUAykwTdDCr+I0per6Y6
 github.com/go-git/go-git/v5 v5.4.2 h1:BXyZu9t0VkbiHtqrsvdq39UDhGJTl1h55VW6CSC4aY4=
 github.com/go-git/go-git/v5 v5.4.2/go.mod h1:gQ1kArt6d+n+BGd+/B/I74HwRTLhth2+zti4ihgckDc=
 github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
-github.com/go-logr/logr v1.2.3 h1:2DntVwHkVopvECVRSlL5PSo9eG+cAkDCuckLubN+rq0=
-github.com/go-logr/logr v1.2.3/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
+github.com/go-logr/logr v1.2.4 h1:g01GSCwiDw2xSZfjJ2/T9M+S6pFdcNtFYsp+Y43HYDQ=
+github.com/go-logr/logr v1.2.4/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
 github.com/go-logr/zapr v1.2.3 h1:a9vnzlIBPQBBkeaR9IuMUfmVOrQlkoC4YfPoFkX3T7A=
 github.com/go-logr/zapr v1.2.3/go.mod h1:eIauM6P8qSvTw5o2ez6UEAfGjQKrxQTl5EoK+Qa2oG4=
 github.com/go-task/slim-sprig v0.0.0-20210107165309-348f09dbbbc0/go.mod h1:fyg7847qk6SyHyPtNmDHnmrv/HOrqktSC+C9fM+CJOE=
@@ -253,3 +253,5 @@ gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY=
 gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ=
 gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
 gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
+k8s.io/utils v0.0.0-20230406110748-d93618cff8a2 h1:qY1Ad8PODbnymg2pRbkyMT/ylpTrCM8P2RJ0yroCyIk=
+k8s.io/utils v0.0.0-20230406110748-d93618cff8a2/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
diff --git a/stash/auth.go b/stash/auth.go
@@ -18,6 +18,7 @@ package stash
 
 import (
 	"errors"
+	"fmt"
 	"net/url"
 
 	"github.com/fluxcd/go-git-providers/gitprovider"
@@ -33,13 +34,13 @@ func NewStashClient(username, token string, optFns ...gitprovider.ClientOption)
 
 	opts, err := gitprovider.MakeClientOptions(optFns...)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed making client options: %w", err)
 	}
 
 	// Create a *http.Client using the transport chain
 	client, err := gitprovider.BuildClientFromTransportChain(opts.GetTransportChain())
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed building client: %w", err)
 	}
 
 	if opts.Domain == nil {
@@ -55,7 +56,7 @@ func NewStashClient(username, token string, optFns ...gitprovider.ClientOption)
 
 	url, err = url.Parse(host)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed parsing host URL %q: %w", host, err)
 	}
 
 	var stashClient *Client
@@ -66,7 +67,7 @@ func NewStashClient(username, token string, optFns ...gitprovider.ClientOption)
 	}
 
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed creating client: %w", err)
 	}
 
 	// By default, turn destructive actions off. But allow overrides.

diff --git a/stash/client.go b/stash/client.go
@@ -173,10 +173,6 @@ func NewClient(httpClient *http.Client, host string, header *http.Header, logger
 		return nil, errors.New("host is required")
 	}
 
-	if (logr.Logger{} == logger) {
-		return nil, errors.New("logger is required")
-	}
-
 	if httpClient == nil {
 		httpClient = &http.Client{
 			Transport: defaultTransport,

diff --git a/stash/client_test.go b/stash/client_test.go
@@ -33,6 +33,7 @@ import (
 	"github.com/go-logr/logr"
 	"github.com/go-logr/zapr"
 	"go.uber.org/zap/zaptest"
+	"k8s.io/utils/pointer"
 )
 
 func Test_NewClient(t *testing.T) {
@@ -45,6 +46,7 @@ func Test_NewClient(t *testing.T) {
 		transport http.RoundTripper
 		log       logr.Logger
 		output    string
+		err       *string
 	}{
 		{
 			name:      "no host",
@@ -54,7 +56,7 @@ func Test_NewClient(t *testing.T) {
 			client:    &http.Client{},
 			transport: defaultTransport,
 			log:       logr.Discard(),
-			output:    "host is required",
+			err:       pointer.String("host is required"),
 		},
 		{
 			name:      "wrong host",
@@ -64,7 +66,7 @@ func Test_NewClient(t *testing.T) {
 			client:    &http.Client{},
 			transport: defaultTransport,
 			log:       logr.Discard(),
-			output:    `failed to parse host https://local#.@*dev to url, parse "https://local#.@*dev": net/url: invalid userinfo`,
+			err:       pointer.String(`failed to parse host https://local#.@*dev to url, parse "https://local#.@*dev": net/url: invalid userinfo`),
 		},
 		{
 			name:   "default host",
@@ -85,7 +87,6 @@ func Test_NewClient(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			var val string
 			c, err := NewClient(&http.Client{}, tt.host, tt.header, tt.log, func(c *Client) error {
 				c.Client.HTTPClient = &http.Client{
 					Transport: tt.transport,
@@ -94,13 +95,18 @@ func Test_NewClient(t *testing.T) {
 				return nil
 			})
 			if err != nil {
-				val = fmt.Sprintf("%s", err)
-			}
-			if c != nil {
-				val = c.BaseURL.String()
-			}
-			if val != tt.output {
-				t.Errorf("Expected %s, got %s", tt.output, val)
+				if tt.err == nil {
+					t.Errorf("did not expect an error but got %q", err)
+				} else if *tt.err != err.Error() {
+					t.Errorf("unexpected error: expected %q but got %q", *tt.err, err)
+				}
+			} else {
+				if tt.err != nil {
+					t.Errorf("expected error %q but got none", *tt.err)
+				}
+				if tt.output != c.BaseURL.String() {
+					t.Errorf("expected output %q but got %q", tt.output, c.BaseURL.String())
+				}
 			}
 		})
 	}