Fix tests and refactor logic for FindParentTags

Author: David Collom

cmd/app/options.go

@@ -81,6 +81,11 @@ type Options struct {
 	DefaultTestAll bool
 }
 
+type envMatcher struct {
+	re     *regexp.Regexp
+	action func(matches []string, value string)
+}
+
 func (o *Options) addFlags(cmd *cobra.Command) {
 	var nfs cliflag.NamedFlagSets
 
@@ -364,56 +369,81 @@ func (o *Options) assignSelfhosted(envs []string) {
 	}
 
 	initOptions := func(name string) {
+		if name == "" {
+			panic("Not meant to be empty!")
+		}
 		if o.Client.Selfhosted[name] == nil {
 			o.Client.Selfhosted[name] = new(selfhosted.Options)
 		}
 	}
 
-	regexActions := map[*regexp.Regexp]func(matches []string, value string){
-		selfhostedHostReg: func(matches []string, value string) {
-			initOptions(matches[1])
-			o.Client.Selfhosted[matches[1]].Host = value
+	// Go maps iterate in random order - Using a slice to consistency
+	regexActions := []envMatcher{
+		{
+			re: selfhostedTokenPath,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				o.Client.Selfhosted[matches[1]].TokenPath = value
+			},
 		},
-		selfhostedUsernameReg: func(matches []string, value string) {
-			initOptions(matches[1])
-			o.Client.Selfhosted[matches[1]].Username = value
+		{
+			re: selfhostedTokenReg,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				o.Client.Selfhosted[matches[1]].Bearer = value
+			},
 		},
-		selfhostedPasswordReg: func(matches []string, value string) {
-			initOptions(matches[1])
-			o.Client.Selfhosted[matches[1]].Password = value
+		// All your other patterns (host, username, password, insecure, capath...)
+		{
+			re: selfhostedHostReg,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				o.Client.Selfhosted[matches[1]].Host = value
+			},
 		},
-		selfhostedTokenPath: func(matches []string, value string) {
-			initOptions(matches[1])
-			o.Client.Selfhosted[matches[1]].TokenPath = value
+		{
+			re: selfhostedUsernameReg,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				o.Client.Selfhosted[matches[1]].Username = value
+			},
 		},
-		selfhostedTokenReg: func(matches []string, value string) {
-			initOptions(matches[1])
-			o.Client.Selfhosted[matches[1]].Bearer = value
+		{
+			re: selfhostedPasswordReg,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				o.Client.Selfhosted[matches[1]].Password = value
+			},
 		},
-		selfhostedInsecureReg: func(matches []string, value string) {
-			initOptions(matches[1])
-			if val, err := strconv.ParseBool(value); err == nil {
-				o.Client.Selfhosted[matches[1]].Insecure = val
-			}
+		{
+			re: selfhostedInsecureReg,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				if b, err := strconv.ParseBool(value); err == nil {
+					o.Client.Selfhosted[matches[1]].Insecure = b
+				}
+			},
 		},
-		selfhostedCAPath: func(matches []string, value string) {
-			initOptions(matches[1])
-			o.Client.Selfhosted[matches[1]].CAPath = value
+		{
+			re: selfhostedCAPath,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				o.Client.Selfhosted[matches[1]].CAPath = value
+			},
 		},
 	}
 
 	for _, env := range envs {
-		pair := strings.SplitN(env, "=", 2)
-		if len(pair) != 2 || len(pair[1]) == 0 {
+		parts := strings.SplitN(env, "=", 2)
+		if len(parts) != 2 || parts[1] == "" {
 			continue
 		}
+		key := strings.ToUpper(parts[0])
+		val := parts[1]
 
-		key := strings.ToUpper(pair[0])
-		value := pair[1]
-
-		for regex, action := range regexActions {
-			if matches := regex.FindStringSubmatch(key); len(matches) == 2 {
-				action(matches, value)
+		for _, p := range regexActions {
+			if match := p.re.FindStringSubmatch(key); len(match) == 2 {
+				p.action(match, val)
 				break
 			}
 		}

pkg/client/acr/acr.go

@@ -94,14 +94,7 @@ func (c *Client) Tags(ctx context.Context, host, repo, image string) ([]api.Imag
 			current := base   // copy the base
 			current.Tag = tag // set tag value
 
-			// Already exists — add as child
-			if parent, exists := tags[tag]; exists {
-				parent.Children = append(parent.Children, &current)
-				tags[tag] = parent
-			} else {
-				// First occurrence — assign as root
-				tags[tag] = current
-			}
+			util.FindParentTags(tags, tag, &current)
 		}
 	}
 	return util.TagMaptoList(tags), nil

pkg/client/client_test.go

@@ -2,10 +2,10 @@ package client
 
 import (
 	"context"
-	"reflect"
 	"testing"
 
 	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
 
 	"github.com/jetstack/version-checker/pkg/api"
 	"github.com/jetstack/version-checker/pkg/client/acr"
@@ -181,20 +181,10 @@ func TestFromImageURL(t *testing.T) {
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
 			client, host, path := handler.fromImageURL(test.url)
-			if reflect.TypeOf(client) != reflect.TypeOf(test.expClient) {
-				t.Errorf("unexpected client, exp=%v got=%v",
-					reflect.TypeOf(test.expClient), reflect.TypeOf(client))
-			}
 
-			if host != test.expHost {
-				t.Errorf("unexpected host, exp=%v got=%v",
-					test.expHost, host)
-			}
-
-			if path != test.expPath {
-				t.Errorf("unexpected path, exp=%s got=%s",
-					test.expPath, path)
-			}
+			assert.IsType(t, test.expClient, client)
+			assert.Equal(t, test.expHost, host)
+			assert.Equal(t, test.expPath, path)
 		})
 	}
 }

pkg/client/ecr/ecr.go

@@ -84,14 +84,7 @@ func (c *Client) Tags(ctx context.Context, host, repo, image string) ([]api.Imag
 			current := base   // copy the base
 			current.Tag = tag // set tag value
 
-			// Already exists — add as child
-			if parent, exists := tags[tag]; exists {
-				parent.Children = append(parent.Children, &current)
-				tags[tag] = parent
-			} else {
-				// First occurrence — assign as root
-				tags[tag] = current
-			}
+			util.FindParentTags(tags, tag, &current)
 		}
 	}
 

pkg/client/selfhosted/path.go

@@ -13,6 +13,9 @@ const (
 )
 
 func (c *Client) IsHost(host string) bool {
+	if c.hostRegex == nil {
+		return c.Host == host
+	}
 	return c.hostRegex.MatchString(host)
 }
 

pkg/client/selfhosted/path_test.go

@@ -81,8 +81,25 @@ func TestIsHost(t *testing.T) {
 			assert.Equal(t, test.expIs,
 				handler.IsHost(test.host),
 			)
+			assert.Equal(t, test.expIs, handler.IsHost(test.host))
 		})
 	}
+
+	// t.Run("No Options set on client", func(t *testing.T) {
+	// 	handler, err := New(context.TODO(), logrus.NewEntry(logrus.New()), &Options{})
+	// 	require.NoError(t, err)
+
+	// 	assert.NotPanics(t, func() { handler.IsHost("example.com") })
+	// })
+
+	// t.Run("No Options set on client", func(t *testing.T) {
+
+	// 	handler := &Client{Options: &Options{Host: "abc"}}
+	// 	require.NoError(t, err)
+
+	// 	assert.NotPanics(t, func() { handler.IsHost("example.com") })
+	// 	assert.True(t, handler.IsHost("abc"))
+	// })
 }
 
 func TestRepoImage(t *testing.T) {

pkg/client/selfhosted/selfhosted.go

@@ -52,7 +52,6 @@ type Options struct {
 	CAPath    string
 	Insecure  bool
 }
-
 type Client struct {
 	*http.Client
 	*Options
@@ -64,10 +63,6 @@ type Client struct {
 }
 
 func New(ctx context.Context, log *logrus.Entry, opts *Options) (*Client, error) {
-	if opts.Host == "" {
-		return nil, errors.New("host cannot be empty for selfhosted client(s)")
-	}
-
 	client := &Client{
 		Client: &http.Client{
 			Timeout:   time.Second * 10,
@@ -222,14 +217,8 @@ func (c *Client) Tags(ctx context.Context, host, repo, image string) ([]api.Imag
 			Timestamp:    timestamp,
 			Architecture: api.Architecture(manifestResponse.Architecture),
 		}
-		// Already exists — add as child
-		if parent, exists := tags[tag]; exists {
-			parent.Children = append(parent.Children, &current)
-			tags[tag] = parent
-		} else {
-			// First occurrence — assign as root
-			tags[tag] = current
-		}
+
+		util.FindParentTags(tags, tag, &current)
 
 		for _, manifest := range manifestListResponse.Manifests {
 
@@ -239,14 +228,7 @@ func (c *Client) Tags(ctx context.Context, host, repo, image string) ([]api.Imag
 				current.SHA = manifest.Digest
 			}
 
-			// Already exists — add as child
-			if parent, exists := tags[tag]; exists {
-				parent.Children = append(parent.Children, &current)
-				tags[tag] = parent
-			} else {
-				// First occurrence — assign as root
-				tags[tag] = current
-			}
+			util.FindParentTags(tags, tag, &current)
 		}
 	}
 	return util.TagMaptoList(tags), nil

pkg/client/selfhosted/selfhosted_test.go

@@ -59,16 +59,16 @@ func TestNew(t *testing.T) {
 		assert.Contains(t, err.Error(), "failed parsing url")
 	})
 
-	t.Run("Error on missing host", func(t *testing.T) {
-		opts := &Options{
-			Host:   "",
-			CAPath: "invalid/path",
-		}
-		client, err := New(ctx, log, opts)
-		assert.Nil(t, client)
-		assert.Error(t, err)
-		assert.Contains(t, err.Error(), "host cannot be empty")
-	})
+	// t.Run("Error on missing host", func(t *testing.T) {
+	// 	opts := &Options{
+	// 		Host:   "",
+	// 		CAPath: "invalid/path",
+	// 	}
+	// 	client, err := New(ctx, log, opts)
+	// 	assert.Nil(t, client)
+	// 	assert.Error(t, err)
+	// 	assert.Contains(t, err.Error(), "host cannot be empty")
+	// })
 
 	t.Run("error on username/password and bearer token both specified", func(t *testing.T) {
 		opts := &Options{

pkg/client/util/http_backoff_limiter_test.go

@@ -0,0 +1,65 @@
+package util
+
+import (
+	"testing"
+	"time"
+
+	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
+	"golang.org/x/time/rate"
+)
+
+func TestRateLimitedBackoffLimiter(t *testing.T) {
+	logger := logrus.NewEntry(logrus.New())
+	maxWait := 5 * time.Second
+
+	t.Run("default backoff delay is used when rate limiter allows immediate execution", func(t *testing.T) {
+		limiter := rate.NewLimiter(rate.Every(1*time.Millisecond), 1)
+		backoff := RateLimitedBackoffLimiter(logger, limiter, maxWait)
+
+		min := 100 * time.Millisecond
+		max := 200 * time.Millisecond
+		attemptNum := 1
+
+		delay := backoff(min, max, attemptNum, nil)
+		assert.GreaterOrEqual(t, delay, min)
+		assert.LessOrEqual(t, delay, max)
+	})
+
+	t.Run("rate limiter delay is used when it exceeds default backoff", func(t *testing.T) {
+		limiter := rate.NewLimiter(rate.Every(500*time.Millisecond), 1)
+		backoff := RateLimitedBackoffLimiter(logger, limiter, maxWait)
+
+		min := 100 * time.Millisecond
+		max := 500 * time.Millisecond
+		attemptNum := 3
+
+		delay := backoff(min, max, attemptNum, nil)
+		assert.GreaterOrEqual(t, delay, 500*time.Millisecond)
+		assert.LessOrEqual(t, delay, maxWait)
+	})
+
+	t.Run("maxWait is used when delay exceeds maxWait", func(t *testing.T) {
+		limiter := rate.NewLimiter(rate.Every(10*time.Second), 1)
+		backoff := RateLimitedBackoffLimiter(logger, limiter, maxWait)
+
+		min := maxWait - time.Second
+		max := maxWait
+		attemptNum := 3
+
+		delay := backoff(min, max, attemptNum, nil)
+		assert.Equal(t, maxWait, delay)
+	})
+
+	t.Run("rate limiter reservation fails", func(t *testing.T) {
+		limiter := rate.NewLimiter(rate.Every(1*time.Second), 0) // No tokens available
+		backoff := RateLimitedBackoffLimiter(logger, limiter, maxWait)
+
+		min := 100 * time.Millisecond
+		max := 500 * time.Millisecond
+		attemptNum := 3
+
+		delay := backoff(min, max, attemptNum, nil)
+		assert.Equal(t, maxWait, delay)
+	})
+}

pkg/client/util/util.go

@@ -80,3 +80,14 @@ func TagMaptoList(tags map[string]api.ImageTag) []api.ImageTag {
 	}
 	return taglist
 }
+
+func FindParentTags(tags map[string]api.ImageTag, tag string, current *api.ImageTag) {
+	// Already exists — add as child
+	if parent, exists := tags[tag]; exists {
+		parent.Children = append(parent.Children, current)
+		tags[tag] = parent
+	} else {
+		// First occurrence — assign as root
+		tags[tag] = *current
+	}
+}