Skip to content
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

Support using regex capture groups to clean up the semver of a tag #407

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+108 −37
Open

Support using regex capture groups to clean up the semver of a tag #407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 15 additions & 7 deletions pkg/image/matchfunc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,29 @@ import (
)

// MatchFuncAny matches any pattern, i.e. always returns true
func MatchFuncAny(tagName string, args interface{}) bool {
return true
func MatchFuncAny(tagName string, args interface{}) (string, bool) {
return tagName, true
}

// MatchFuncNone matches no pattern, i.e. always returns false
func MatchFuncNone(tagName string, args interface{}) bool {
return false
func MatchFuncNone(tagName string, args interface{}) (string, bool) {
return tagName, false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should return "" instead of tagName, for consistency with MatchFuncRegexp

}

// MatchFuncRegexp matches the tagName against regexp pattern and returns the result
func MatchFuncRegexp(tagName string, args interface{}) bool {
func MatchFuncRegexp(tagName string, args interface{}) (string, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The methods' documentary comment should be updated to reflect the new behavior.

pattern, ok := args.(*regexp.Regexp)
if !ok {
log.Errorf("args is not a RegExp")
return false
return tagName, false
}
matches := pattern.FindStringSubmatch(tagName)
switch len(matches) {
case 0:
return "", false
case 1:
return tagName, true
default:
return matches[1], true
}
return pattern.Match([]byte(tagName))
}
18 changes: 13 additions & 5 deletions pkg/image/matchfunc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,28 @@ import (
)

func Test_MatchFuncAny(t *testing.T) {
assert.True(t, MatchFuncAny("whatever", nil))
result, ok := MatchFuncAny("whatever", nil)
assert.True(t, ok)
assert.Equal(t, "whatever", result)
}

func Test_MatchFuncNone(t *testing.T) {
assert.False(t, MatchFuncNone("whatever", nil))
_, ok := MatchFuncNone("whatever", nil)
assert.False(t, ok)
}

func Test_MatchFuncRegexp(t *testing.T) {
t.Run("Test with valid expression", func(t *testing.T) {
re := regexp.MustCompile("[a-z]+")
assert.True(t, MatchFuncRegexp("lemon", re))
assert.False(t, MatchFuncRegexp("31337", re))
result, ok := MatchFuncRegexp("lemon", re)
assert.True(t, ok)
assert.Equal(t, "lemon", result)

_, ok = MatchFuncRegexp("31337", re)
assert.False(t, ok)
})
t.Run("Test with invalid type", func(t *testing.T) {
assert.False(t, MatchFuncRegexp("lemon", "[a-z]+"))
_, ok := MatchFuncRegexp("lemon", "[a-z]+")
assert.False(t, ok)
})
}
21 changes: 16 additions & 5 deletions pkg/image/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,9 @@ func Test_GetMatchOption(t *testing.T) {
}
img := NewFromIdentifier("dummy=foo/bar:1.12")
matchFunc, matchArgs := img.GetParameterMatch(annotations)
_, ok := matchFunc("", nil)
require.NotNil(t, matchFunc)
require.Equal(t, false, matchFunc("", nil))
require.Equal(t, false, ok)
assert.Nil(t, matchArgs)
})

Expand All @@ -182,8 +183,13 @@ func Test_GetMatchOption(t *testing.T) {
require.NotNil(t, matchFunc)
require.NotNil(t, matchArgs)
assert.IsType(t, &regexp.Regexp{}, matchArgs)
assert.True(t, matchFunc("0.0.1", matchArgs))
assert.False(t, matchFunc("v0.0.1", matchArgs))

result, ok := matchFunc("0.0.1", matchArgs)
assert.True(t, ok)
assert.Equal(t, "0.0.1", result)

_, ok = matchFunc("v0.0.1", matchArgs)
assert.False(t, ok)
})

t.Run("Get match option from application-wide annotation", func(t *testing.T) {
Expand All @@ -195,8 +201,13 @@ func Test_GetMatchOption(t *testing.T) {
require.NotNil(t, matchFunc)
require.NotNil(t, matchArgs)
assert.IsType(t, &regexp.Regexp{}, matchArgs)
assert.False(t, matchFunc("0.0.1", matchArgs))
assert.True(t, matchFunc("v0.0.1", matchArgs))

_, ok := matchFunc("0.0.1", matchArgs)
assert.False(t, ok)

result, ok := matchFunc("v0.0.1", matchArgs)
assert.True(t, ok)
assert.Equal(t, "v0.0.1", result)
})
}

Expand Down
9 changes: 4 additions & 5 deletions pkg/image/version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ type VersionConstraint struct {
Options *options.ManifestOptions
}

type MatchFuncFn func(tagName string, pattern interface{}) bool
type MatchFuncFn func(tagName string, pattern interface{}) (string, bool)

// String returns the string representation of VersionConstraint
func (vc *VersionConstraint) String() string {
Expand Down Expand Up @@ -132,17 +132,16 @@ func (img *ContainerImage) GetNewestVersionFromTags(vc *VersionConstraint, tagLi

if vc.Strategy == StrategySemVer {
// Non-parseable tag does not mean error - just skip it
ver, err := semver.NewVersion(tag.TagName)
if err != nil {
if tag.TagVersion == nil {
logCtx.Tracef("Not a valid version: %s", tag.TagName)
continue
}

// If we have a version constraint, check image tag against it. If the
// constraint is not satisfied, skip tag.
if semverConstraint != nil {
if !semverConstraint.Check(ver) {
logCtx.Tracef("%s did not match constraint %s", ver.Original(), vc.Constraint)
if !semverConstraint.Check(tag.TagVersion) {
logCtx.Tracef("%s did not match constraint %s", tag.TagNamePart, vc.Constraint)
continue
}
}
Expand Down
36 changes: 29 additions & 7 deletions pkg/registry/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,11 @@ func (endpoint *RegistryEndpoint) GetTags(img *image.ContainerImage, regClient R
return nil, err
}

tags := []string{}
type tuple struct {
original, found string
}

tags := []tuple{}

// For digest strategy, we do require a version constraint
if vc.Strategy.NeedsVersionConstraint() && vc.Constraint == "" {
Expand All @@ -62,14 +66,32 @@ func (endpoint *RegistryEndpoint) GetTags(img *image.ContainerImage, regClient R
// digest, all but the constraint tag are ignored.
if vc.MatchFunc != nil || len(vc.IgnoreList) > 0 || vc.Strategy.WantsOnlyConstraintTag() {
for _, t := range tTags {
if (vc.MatchFunc != nil && !vc.MatchFunc(t, vc.MatchArgs)) || vc.IsTagIgnored(t) || (vc.Strategy.WantsOnlyConstraintTag() && t != vc.Constraint) {
var (
shouldRemove = false
item = tuple{t, t}
)
Comment on lines +69 to +72
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use var() block for consistency with rest of the code. I'd suggest using := operator.

if vc.MatchFunc != nil {
if found, ok := vc.MatchFunc(t, vc.MatchArgs); ok {
item.found = found
} else {
shouldRemove = true
}
}

if vc.IsTagIgnored(t) || (vc.Strategy.WantsOnlyConstraintTag() && t != vc.Constraint) {
shouldRemove = true
}

if shouldRemove {
logCtx.Tracef("Removing tag %s because it either didn't match defined pattern or is ignored", t)
} else {
tags = append(tags, t)
tags = append(tags, item)
}
}
} else {
tags = tTags
for _, t := range tTags {
tags = append(tags, tuple{t, t})
}
}

// In some cases, we don't need to fetch the metadata to get the creation time
Expand All @@ -88,7 +110,7 @@ func (endpoint *RegistryEndpoint) GetTags(img *image.ContainerImage, regClient R
} else if endpoint.TagListSort == TagListSortLatestLast {
ts = i
}
imgTag := tag.NewImageTag(tagStr, time.Unix(int64(ts), 0), "")
imgTag := tag.NewImageTagWithTagPart(tagStr.original, tagStr.found, time.Unix(int64(ts), 0), "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I think tagStr should be renamed to tagInfo or something like that in the loop to better reflect the fact that it's not a string anymore. Using tagStr was a bad decision in the first place, but now it just becomes confusing :)

tagList.Add(imgTag)
}
return tagList, nil
Expand All @@ -109,7 +131,7 @@ func (endpoint *RegistryEndpoint) GetTags(img *image.ContainerImage, regClient R
// an error, we treat it as a cache miss and just go ahead to invalidate
// the entry.
if vc.Strategy.IsCacheable() {
imgTag, err := endpoint.Cache.GetTag(nameInRegistry, tagStr)
imgTag, err := endpoint.Cache.GetTag(nameInRegistry, tagStr.original)
if err != nil {
log.Warnf("invalid entry for %s:%s in cache, invalidating.", nameInRegistry, imgTag.TagName)
} else if imgTag != nil {
Expand Down Expand Up @@ -172,7 +194,7 @@ func (endpoint *RegistryEndpoint) GetTags(img *image.ContainerImage, regClient R
tagList.Add(imgTag)
tagListLock.Unlock()
endpoint.Cache.SetTag(nameInRegistry, imgTag)
}(tagStr)
}(tagStr.original)
}

wg.Wait()
Expand Down
39 changes: 31 additions & 8 deletions pkg/tag/tag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ import (
// ImageTag is a representation of an image tag with metadata
// Use NewImageTag to to initialize a new object.
type ImageTag struct {
TagName string
TagDate *time.Time
TagDigest string
TagName string
TagNamePart string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this field would need some additional comment on what it actually represents, it is not clear to me.

TagDate *time.Time
TagDigest string
TagVersion *semver.Version
}

// ImageTagList is a collection of ImageTag objects.
Expand Down Expand Up @@ -47,13 +49,34 @@ func (il SortableImageTagList) Swap(i, j int) {

// NewImageTag initializes an ImageTag object and returns it
func NewImageTag(tagName string, tagDate time.Time, tagDigest string) *ImageTag {
return NewImageTagWithTagPart(tagName, tagName, tagDate, tagDigest)
}

// NewImageTagWithTagPart initializes an ImageTag object and returns it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be more specific what the difference to NewImageTag is. Also I was wondering if this should be rather something more along the lines of the following, instead of a new initializer:

func (t *ImageTag) WithTagNamePart(tagNamePart string) *ImageTag {
  t.TagNamePart = tagNamePart
  return t
}

tag := NewImageTag(...).WithTagNamePart(...)

WDYT?

func NewImageTagWithTagPart(tagName, tagNamePart string, tagDate time.Time, tagDigest string) *ImageTag {
tag := &ImageTag{}
tag.TagName = tagName
tag.TagNamePart = tagNamePart
tag.TagDate = &tagDate
tag.TagDigest = tagDigest
tag.TagVersion = tryParseSemVer(tagNamePart)
return tag
}

func tryParseSemVer(tag string) *semver.Version {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please have a unit test for this method?

if len(tag) > 0 && tag[0] != 'v' {
tag = "v" + tag
}
Comment on lines +67 to +69
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is necessary. I think Masterminds/semver treats "x.y.z" and "vx.y.z" the same.


svi, err := semver.NewVersion(tag)
if err != nil {
log.Debugf("could not parse input tag %s as semver: %v", tag, err)
return nil
}

return svi
}

// NewImageTagList initializes an ImageTagList object and returns it
func NewImageTagList() *ImageTagList {
itl := ImageTagList{}
Expand Down Expand Up @@ -157,17 +180,17 @@ func (il ImageTagList) SortBySemVer() SortableImageTagList {

sil := SortableImageTagList{}
svl := make([]*semver.Version, 0)
tagMap := make(map[string]*ImageTag)
for _, v := range il.items {
svi, err := semver.NewVersion(v.TagName)
if err != nil {
log.Debugf("could not parse input tag %s as semver: %v", v.TagName, err)
if v.TagVersion == nil {
continue
}
svl = append(svl, svi)
tagMap[v.TagVersion.Original()] = v
svl = append(svl, v.TagVersion)
}
sort.Sort(semver.Collection(svl))
for _, svi := range svl {
sil = append(sil, NewImageTag(svi.Original(), *il.items[svi.Original()].TagDate, il.items[svi.Original()].TagDigest))
sil = append(sil, tagMap[svi.Original()])
}
return sil
}
Expand Down