Support using regex capture groups to clean up the semver of a tag #407
Conversation
|
|
There was a problem hiding this comment.
Thanks @djeebus !
This indeed looks useful. However, I'm not so sure whether it's a good idea to reuse allowed-tags for transforming the tag name to a version though, even if it may seem reasonable and elegant currently. But it would not allow us to use multiple regexps in the future for allow-tags, or introduce a simpler matcher (e.g. a glob) to be combined with allowed-tags in a semver update strategy.
I think we may want to introduce a new annotation, e.g. semver-transformer that will be used for this transformation.
I'm happy to discuss the pros and cons of both before you make a change, tho.
| TagDate *time.Time | ||
| TagDigest string | ||
| TagName string | ||
| TagNamePart string |
There was a problem hiding this comment.
I think this field would need some additional comment on what it actually represents, it is not clear to me.
| return tag | ||
| } | ||
|
|
||
| func tryParseSemVer(tag string) *semver.Version { |
There was a problem hiding this comment.
Can we please have a unit test for this method?
| if len(tag) > 0 && tag[0] != 'v' { | ||
| tag = "v" + tag | ||
| } |
There was a problem hiding this comment.
I'm not sure this is necessary. I think Masterminds/semver treats "x.y.z" and "vx.y.z" the same.
| var ( | ||
| shouldRemove = false | ||
| item = tuple{t, t} | ||
| ) |
There was a problem hiding this comment.
Please don't use var() block for consistency with rest of the code. I'd suggest using := operator.
| } | ||
|
|
||
| // 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) { |
There was a problem hiding this comment.
The methods' documentary comment should be updated to reflect the new behavior.
| func MatchFuncNone(tagName string, args interface{}) bool { | ||
| return false | ||
| func MatchFuncNone(tagName string, args interface{}) (string, bool) { | ||
| return tagName, false |
There was a problem hiding this comment.
Should return "" instead of tagName, for consistency with MatchFuncRegexp
| @@ -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), "") | |||
There was a problem hiding this comment.
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 :)
| return NewImageTagWithTagPart(tagName, tagName, tagDate, tagDigest) | ||
| } | ||
|
|
||
| // NewImageTagWithTagPart initializes an ImageTag object and returns it |
There was a problem hiding this comment.
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?
|
@jannfis thanks for the review! I'm certainly not opposed to creating a new tag. It would increase the scope a little bit, but if it's better for you guys, I can swap to that when I get a chance. Just so we're on the same page, something like the following would match Edit: maybe w/ the |
|
The For example, if you want to stay within 1.2 minor version, you'd configure I would suggest the following for this change:
|
This allows argocd-image-updater to support finding the latest tag even when they don't really follow semver exactly. Some good examples of this:
1.25.7.5604-980a13e02(semver requires the final character to be a+, not-)version-5.6.16(leading chars should bev, notversion-)This PR would let us set the
regexpto be (in pms-docker's case)^(\d+\.\d+\.\d+)\.\d+-.*$', or just(\d+\.\d+\.\d+). The code checks for groups, and if none are found, assumes that the full tag is the semver (same as current behavior). If a capture group is found, however, then it parses the capture group as a semver, rather than the full thing.Please let me know if there's anything I can do to help this along, or if I'm out-of-scope for this project, and I'll figure something out. This would help me keep a lot of my homelab images up to date though. Thanks!!