Skip to content

Commit 94db5c2

Browse files
davidcollomdavidcollom
committed
Fix tests and refactor logic for FindParentTags
1 parent ea8bf67 commit 94db5c2

File tree

14 files changed

+238
-151
lines changed

14 files changed

+238
-151
lines changed

‎cmd/app/options.go

Lines changed: 62 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,11 @@ type Options struct {
8181
DefaultTestAll bool
8282
}
8383

84+
type envMatcher struct {
85+
re *regexp.Regexp
86+
action func(matches []string, value string)
87+
}
88+
8489
func (o *Options) addFlags(cmd *cobra.Command) {
8590
var nfs cliflag.NamedFlagSets
8691

@@ -364,56 +369,81 @@ func (o *Options) assignSelfhosted(envs []string) {
364369
}
365370

366371
initOptions := func(name string) {
372+
if name == "" {
373+
panic("Not meant to be empty!")
374+
}
367375
if o.Client.Selfhosted[name] == nil {
368376
o.Client.Selfhosted[name] = new(selfhosted.Options)
369377
}
370378
}
371379

372-
regexActions := map[*regexp.Regexp]func(matches []string, value string){
373-
selfhostedHostReg: func(matches []string, value string) {
374-
initOptions(matches[1])
375-
o.Client.Selfhosted[matches[1]].Host = value
380+
// Go maps iterate in random order - Using a slice to consistency
381+
regexActions := []envMatcher{
382+
{
383+
re: selfhostedTokenPath,
384+
action: func(matches []string, value string) {
385+
initOptions(matches[1])
386+
o.Client.Selfhosted[matches[1]].TokenPath = value
387+
},
376388
},
377-
selfhostedUsernameReg: func(matches []string, value string) {
378-
initOptions(matches[1])
379-
o.Client.Selfhosted[matches[1]].Username = value
389+
{
390+
re: selfhostedTokenReg,
391+
action: func(matches []string, value string) {
392+
initOptions(matches[1])
393+
o.Client.Selfhosted[matches[1]].Bearer = value
394+
},
380395
},
381-
selfhostedPasswordReg: func(matches []string, value string) {
382-
initOptions(matches[1])
383-
o.Client.Selfhosted[matches[1]].Password = value
396+
// All your other patterns (host, username, password, insecure, capath...)
397+
{
398+
re: selfhostedHostReg,
399+
action: func(matches []string, value string) {
400+
initOptions(matches[1])
401+
o.Client.Selfhosted[matches[1]].Host = value
402+
},
384403
},
385-
selfhostedTokenPath: func(matches []string, value string) {
386-
initOptions(matches[1])
387-
o.Client.Selfhosted[matches[1]].TokenPath = value
404+
{
405+
re: selfhostedUsernameReg,
406+
action: func(matches []string, value string) {
407+
initOptions(matches[1])
408+
o.Client.Selfhosted[matches[1]].Username = value
409+
},
388410
},
389-
selfhostedTokenReg: func(matches []string, value string) {
390-
initOptions(matches[1])
391-
o.Client.Selfhosted[matches[1]].Bearer = value
411+
{
412+
re: selfhostedPasswordReg,
413+
action: func(matches []string, value string) {
414+
initOptions(matches[1])
415+
o.Client.Selfhosted[matches[1]].Password = value
416+
},
392417
},
393-
selfhostedInsecureReg: func(matches []string, value string) {
394-
initOptions(matches[1])
395-
if val, err := strconv.ParseBool(value); err == nil {
396-
o.Client.Selfhosted[matches[1]].Insecure = val
397-
}
418+
{
419+
re: selfhostedInsecureReg,
420+
action: func(matches []string, value string) {
421+
initOptions(matches[1])
422+
if b, err := strconv.ParseBool(value); err == nil {
423+
o.Client.Selfhosted[matches[1]].Insecure = b
424+
}
425+
},
398426
},
399-
selfhostedCAPath: func(matches []string, value string) {
400-
initOptions(matches[1])
401-
o.Client.Selfhosted[matches[1]].CAPath = value
427+
{
428+
re: selfhostedCAPath,
429+
action: func(matches []string, value string) {
430+
initOptions(matches[1])
431+
o.Client.Selfhosted[matches[1]].CAPath = value
432+
},
402433
},
403434
}
404435

405436
for _, env := range envs {
406-
pair := strings.SplitN(env, "=", 2)
407-
if len(pair) != 2 || len(pair[1]) == 0 {
437+
parts := strings.SplitN(env, "=", 2)
438+
if len(parts) != 2 || parts[1] == "" {
408439
continue
409440
}
441+
key := strings.ToUpper(parts[0])
442+
val := parts[1]
410443

411-
key := strings.ToUpper(pair[0])
412-
value := pair[1]
413-
414-
for regex, action := range regexActions {
415-
if matches := regex.FindStringSubmatch(key); len(matches) == 2 {
416-
action(matches, value)
444+
for _, p := range regexActions {
445+
if match := p.re.FindStringSubmatch(key); len(match) == 2 {
446+
p.action(match, val)
417447
break
418448
}
419449
}

‎cmd/app/options_test.go

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ func TestInvalidSelfhostedPanic(t *testing.T) {
199199
}
200200
for name, test := range tests {
201201
t.Run(name, func(t *testing.T) {
202-
defer func() { recover() }()
202+
defer func() { _ = recover() }()
203203

204204
o := new(Options)
205205
o.assignSelfhosted(test.envs)
@@ -234,10 +234,7 @@ func TestInvalidSelfhostedOpts(t *testing.T) {
234234

235235
valid := validSelfHostedOpts(&test.opts)
236236

237-
if !reflect.DeepEqual(test.valid, valid) {
238-
t.Errorf("unexpected selfhosted valid options, exp=%#+v got=%#+v",
239-
test.valid, valid)
240-
}
237+
assert.Equal(t, test.valid, valid)
241238
})
242239
}
243240
}

‎pkg/client/acr/acr.go

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -94,14 +94,7 @@ func (c *Client) Tags(ctx context.Context, host, repo, image string) ([]api.Imag
9494
current := base // copy the base
9595
current.Tag = tag // set tag value
9696

97-
// Already exists — add as child
98-
if parent, exists := tags[tag]; exists {
99-
parent.Children = append(parent.Children, &current)
100-
tags[tag] = parent
101-
} else {
102-
// First occurrence — assign as root
103-
tags[tag] = current
104-
}
97+
util.FindParentTags(tags, tag, &current)
10598
}
10699
}
107100
return util.TagMaptoList(tags), nil

‎pkg/client/client_test.go

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,10 @@ package client
22

33
import (
44
"context"
5-
"reflect"
65
"testing"
76

87
"github.com/sirupsen/logrus"
8+
"github.com/stretchr/testify/assert"
99

1010
"github.com/jetstack/version-checker/pkg/api"
1111
"github.com/jetstack/version-checker/pkg/client/acr"
@@ -181,20 +181,10 @@ func TestFromImageURL(t *testing.T) {
181181
for name, test := range tests {
182182
t.Run(name, func(t *testing.T) {
183183
client, host, path := handler.fromImageURL(test.url)
184-
if reflect.TypeOf(client) != reflect.TypeOf(test.expClient) {
185-
t.Errorf("unexpected client, exp=%v got=%v",
186-
reflect.TypeOf(test.expClient), reflect.TypeOf(client))
187-
}
188184

189-
if host != test.expHost {
190-
t.Errorf("unexpected host, exp=%v got=%v",
191-
test.expHost, host)
192-
}
193-
194-
if path != test.expPath {
195-
t.Errorf("unexpected path, exp=%s got=%s",
196-
test.expPath, path)
197-
}
185+
assert.IsType(t, test.expClient, client)
186+
assert.Equal(t, test.expHost, host)
187+
assert.Equal(t, test.expPath, path)
198188
})
199189
}
200190
}

‎pkg/client/ecr/ecr.go

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -84,14 +84,7 @@ func (c *Client) Tags(ctx context.Context, host, repo, image string) ([]api.Imag
8484
current := base // copy the base
8585
current.Tag = tag // set tag value
8686

87-
// Already exists — add as child
88-
if parent, exists := tags[tag]; exists {
89-
parent.Children = append(parent.Children, &current)
90-
tags[tag] = parent
91-
} else {
92-
// First occurrence — assign as root
93-
tags[tag] = current
94-
}
87+
util.FindParentTags(tags, tag, &current)
9588
}
9689
}
9790

‎pkg/client/selfhosted/path.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@ const (
1313
)
1414

1515
func (c *Client) IsHost(host string) bool {
16+
if c.hostRegex == nil {
17+
return c.Host == host
18+
}
1619
return c.hostRegex.MatchString(host)
1720
}
1821

‎pkg/client/selfhosted/path_test.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,25 @@ func TestIsHost(t *testing.T) {
8181
assert.Equal(t, test.expIs,
8282
handler.IsHost(test.host),
8383
)
84+
assert.Equal(t, test.expIs, handler.IsHost(test.host))
8485
})
8586
}
87+
88+
// t.Run("No Options set on client", func(t *testing.T) {
89+
// handler, err := New(context.TODO(), logrus.NewEntry(logrus.New()), &Options{})
90+
// require.NoError(t, err)
91+
92+
// assert.NotPanics(t, func() { handler.IsHost("example.com") })
93+
// })
94+
95+
// t.Run("No Options set on client", func(t *testing.T) {
96+
97+
// handler := &Client{Options: &Options{Host: "abc"}}
98+
// require.NoError(t, err)
99+
100+
// assert.NotPanics(t, func() { handler.IsHost("example.com") })
101+
// assert.True(t, handler.IsHost("abc"))
102+
// })
86103
}
87104

88105
func TestRepoImage(t *testing.T) {

‎pkg/client/selfhosted/selfhosted.go

Lines changed: 3 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@ type Options struct {
5252
CAPath string
5353
Insecure bool
5454
}
55-
5655
type Client struct {
5756
*http.Client
5857
*Options
@@ -64,10 +63,6 @@ type Client struct {
6463
}
6564

6665
func New(ctx context.Context, log *logrus.Entry, opts *Options) (*Client, error) {
67-
if opts.Host == "" {
68-
return nil, errors.New("host cannot be empty for selfhosted client(s)")
69-
}
70-
7166
client := &Client{
7267
Client: &http.Client{
7368
Timeout: time.Second * 10,
@@ -222,14 +217,8 @@ func (c *Client) Tags(ctx context.Context, host, repo, image string) ([]api.Imag
222217
Timestamp: timestamp,
223218
Architecture: api.Architecture(manifestResponse.Architecture),
224219
}
225-
// Already exists — add as child
226-
if parent, exists := tags[tag]; exists {
227-
parent.Children = append(parent.Children, &current)
228-
tags[tag] = parent
229-
} else {
230-
// First occurrence — assign as root
231-
tags[tag] = current
232-
}
220+
221+
util.FindParentTags(tags, tag, &current)
233222

234223
for _, manifest := range manifestListResponse.Manifests {
235224

@@ -239,14 +228,7 @@ func (c *Client) Tags(ctx context.Context, host, repo, image string) ([]api.Imag
239228
current.SHA = manifest.Digest
240229
}
241230

242-
// Already exists — add as child
243-
if parent, exists := tags[tag]; exists {
244-
parent.Children = append(parent.Children, &current)
245-
tags[tag] = parent
246-
} else {
247-
// First occurrence — assign as root
248-
tags[tag] = current
249-
}
231+
util.FindParentTags(tags, tag, &current)
250232
}
251233
}
252234
return util.TagMaptoList(tags), nil

‎pkg/client/selfhosted/selfhosted_test.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -59,16 +59,16 @@ func TestNew(t *testing.T) {
5959
assert.Contains(t, err.Error(), "failed parsing url")
6060
})
6161

62-
t.Run("Error on missing host", func(t *testing.T) {
63-
opts := &Options{
64-
Host: "",
65-
CAPath: "invalid/path",
66-
}
67-
client, err := New(ctx, log, opts)
68-
assert.Nil(t, client)
69-
assert.Error(t, err)
70-
assert.Contains(t, err.Error(), "host cannot be empty")
71-
})
62+
// t.Run("Error on missing host", func(t *testing.T) {
63+
// opts := &Options{
64+
// Host: "",
65+
// CAPath: "invalid/path",
66+
// }
67+
// client, err := New(ctx, log, opts)
68+
// assert.Nil(t, client)
69+
// assert.Error(t, err)
70+
// assert.Contains(t, err.Error(), "host cannot be empty")
71+
// })
7272

7373
t.Run("error on username/password and bearer token both specified", func(t *testing.T) {
7474
opts := &Options{

‎pkg/client/util/http_backoff_limiter_test.go

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
package util
2+
3+
import (
4+
"testing"
5+
"time"
6+
7+
"github.com/sirupsen/logrus"
8+
"github.com/stretchr/testify/assert"
9+
"golang.org/x/time/rate"
10+
)
11+
12+
func TestRateLimitedBackoffLimiter(t *testing.T) {
13+
logger := logrus.NewEntry(logrus.New())
14+
maxWait := 5 * time.Second
15+
16+
t.Run("default backoff delay is used when rate limiter allows immediate execution", func(t *testing.T) {
17+
limiter := rate.NewLimiter(rate.Every(1*time.Millisecond), 1)
18+
backoff := RateLimitedBackoffLimiter(logger, limiter, maxWait)
19+
20+
min := 100 * time.Millisecond
21+
max := 200 * time.Millisecond
22+
attemptNum := 1
23+
24+
delay := backoff(min, max, attemptNum, nil)
25+
assert.GreaterOrEqual(t, delay, min)
26+
assert.LessOrEqual(t, delay, max)
27+
})
28+
29+
t.Run("rate limiter delay is used when it exceeds default backoff", func(t *testing.T) {
30+
limiter := rate.NewLimiter(rate.Every(500*time.Millisecond), 1)
31+
backoff := RateLimitedBackoffLimiter(logger, limiter, maxWait)
32+
33+
min := 100 * time.Millisecond
34+
max := 500 * time.Millisecond
35+
attemptNum := 3
36+
37+
delay := backoff(min, max, attemptNum, nil)
38+
assert.GreaterOrEqual(t, delay, 500*time.Millisecond)
39+
assert.LessOrEqual(t, delay, maxWait)
40+
})
41+
42+
t.Run("maxWait is used when delay exceeds maxWait", func(t *testing.T) {
43+
limiter := rate.NewLimiter(rate.Every(10*time.Second), 1)
44+
backoff := RateLimitedBackoffLimiter(logger, limiter, maxWait)
45+
46+
min := maxWait - time.Second
47+
max := maxWait
48+
attemptNum := 3
49+
50+
delay := backoff(min, max, attemptNum, nil)
51+
assert.Equal(t, maxWait, delay)
52+
})
53+
54+
t.Run("rate limiter reservation fails", func(t *testing.T) {
55+
limiter := rate.NewLimiter(rate.Every(1*time.Second), 0) // No tokens available
56+
backoff := RateLimitedBackoffLimiter(logger, limiter, maxWait)
57+
58+
min := 100 * time.Millisecond
59+
max := 500 * time.Millisecond
60+
attemptNum := 3
61+
62+
delay := backoff(min, max, attemptNum, nil)
63+
assert.Equal(t, maxWait, delay)
64+
})
65+
}

0 commit comments

Comments
 (0)