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.

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

Image filtering and ordering with semver #1266

Merged
merged 4 commits into from Aug 6, 2018

Conversation

@rndstr
Copy link
Contributor

rndstr commented Aug 1, 2018

Currently, containers can be tagged in manifests to filter what image
tags are considered when doing automated releases. Filtering is done by
specifying a wildcard glob. An optional prefix glob: can be used.

This PR adds support for tag filters based on semantic versioning
by using the prefix semver: instead. Version constraints can be
specified that filter images. Since versions have an implicit ordering
this also changes the way images are sorted when trying to determine the
newest image. For glob filtering this falls back to image creation date.

Closes #706

@rndstr rndstr requested review from squaremo and aaron7 Aug 1, 2018
@rndstr rndstr force-pushed the issue/706-semver-filtering-ordering branch from 54caaba to 7091c03 Aug 1, 2018
@stefanprodan

This comment has been minimized.

Copy link
Member

stefanprodan commented Aug 1, 2018

Does this supports the v0.1.2-alpha1 format? Many images have a v prefix for sem ver, that's because GitHub encourages this convention in the release UI.

Some examples:

@rndstr

This comment has been minimized.

Copy link
Contributor Author

rndstr commented Aug 1, 2018

@stefanprodan it does recognize versions with a v prefix. As for pre-releases, I do think that per spec, the range constraints do ignore them unless mentioned explicitly.
So semver:~1 will ignore 1.2.3-alpha.1 but pick 1.2.2.

@rndstr rndstr force-pushed the issue/706-semver-filtering-ordering branch from 7091c03 to 67ff574 Aug 2, 2018
Copy link
Member

squaremo left a comment

This is a thorough job with some nice refactorings and tidies 💯

I made some comments. Most just offer advice or invite an quick explanation or rebuttal; the only thing I think definitely needs a code change is the accidental change in meaning in policy_cmd.go.

Gopkg.toml Outdated
@@ -47,5 +47,5 @@ required = ["k8s.io/code-generator/cmd/client-gen"]
version = "v1.0.0"

[[constraint]]
branch = "master"

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 2, 2018

Member

Why does this constraint go away?

This comment has been minimized.

Copy link
@rndstr

rndstr Aug 2, 2018

Author Contributor

i don't know. it is still in use, so i re-added it.

@@ -142,7 +142,7 @@ func calculatePolicyChanges(opts *controllerPolicyOpts) (policy.Update, error) {
Add(policy.LockedUser)
}
if opts.tagAll != "" {
add = add.Set(policy.TagAll, "glob:"+opts.tagAll)
add = add.Set(policy.TagAll, policy.PatternAll.String())

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 2, 2018

Member

This isn't a meaning-preserving change -- before, this set the "fake" policy TagAll to the value given for the flag, meaning that all containers should get that filter; now, it sets TagAll to glob:*, meaning all containers get the filter glob:*.

This comment has been minimized.

Copy link
@rndstr

rndstr Aug 2, 2018

Author Contributor

oops, good spot. was blinded by the "all"!

name: "semver",
pattern: "semver:~1",
true: []string{"v1", "1", "1.2", "1.2.3"},
false: []string{"", "latest", "2.0.0", ""},

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 2, 2018

Member

"" is in here twice, am I missing something or is it just a mild distrust of empty strings ..

name: "all",
pattern: "semver:*",
true: []string{"1", "1.0", "v1.0.3"},
false: []string{"", "latest", "2.0.1-alpha.1"},

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 2, 2018

Member

I had to look up why the pre-release is not included (and it's a sensible reason). Should we also test the behaviour when giving pre-releases in the pattern?

if services == nil {
return "*"
return PatternAll
}
policies := services[service]

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 2, 2018

Member

How come services being nil is considered possible, but a service not having an entry isn't?

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 2, 2018

Member

(I realise you are only adapting this code, but you know, since you have now touched it last ...)

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 2, 2018

Member

Actually I'm not sure why this doesn't NPE in the test 🤔
In any case, I think it'd be better to explicitly check for the presence of the policy set, rather than relying on a usable zero value being returned.

This comment has been minimized.

Copy link
@rndstr

rndstr Aug 2, 2018

Author Contributor

Actually I'm not sure why this doesn't NPE in the test

aah, i think since the (s Set) Get() method has an implicit pointer receiver (from map) which makes it fine to call Get() on the nil Set and then accessing a value of that nil map is non-panicky too (no dereferencing)

This comment has been minimized.

Copy link
@rndstr

rndstr Aug 2, 2018

Author Contributor

I added the explicit checks; technically it would work fine if it were just

if pattern, ok := services[service].Get(TagPrefix(container)); ok {
	return NewPattern(pattern)
}
return PatternAll
@@ -231,7 +231,7 @@ func (s ReleaseSpec) calculateImageUpdates(rc ReleaseContext, candidates []*Cont
for _, container := range containers {
currentImageID := container.Image

filteredImages := imageRepos.GetRepoImages(currentImageID.Name).Filter("*")
filteredImages := imageRepos.GetRepoImages(currentImageID.Name).Filter(policy.PatternAll)

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 2, 2018

Member

Wow, we need to change this don't we. Not expecting you to do it here though.

This comment has been minimized.

Copy link
@rndstr

rndstr Aug 2, 2018

Author Contributor

yeah, for now we could just sort the image list returned by GetRepoImages() here instead of redundantly copying the image list in Filter(). but in #1054 we will adjust the param to Filter() to pass in the container tag pattern instead.

return cs.controllers[i].ContainersOrNil()
}

func (cs clusterContainers) Pattern(i int, container string) policy.Pattern {

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 2, 2018

Member

This doesn't look like it's used

}

policyResourceMap, _, err := d.getPolicyResourceMap(ctx)
imageRepos, err := update.FetchImageRepos(d.Registry, clusterContainers{controllers: services, policies: policyResourceMap}, d.Logger)

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 2, 2018

Member

See comment on clusterContainers.Pattern above

// String returns the prefixed string representation.
String() string
// ImageNewerFunc returns a function to compare image newness.
ImageNewerFunc() image.LessFunc

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 2, 2018

Member

This seems like the right place to put the sorting, but the ergonomics seem just slightly off. Let me see if I can explain what I mean .. There's two ways this is used, first as a direct comparison to see which images are newer than the current image, and second as an argument to images.Sort.

In the first case, it'd be neater to just call

if pattern.Newer(a, b) ...

In the second, you could just provide pattern.Newer to the image.Sort procedure (which might make that a bit more concise, and remove the need for image.LessFunc depending on how bound methods are typed, I can't remember).

wdyt?

@@ -38,24 +38,30 @@ func (r ImageRepos) GetRepoImages(repo image.Name) ImageInfos {
// ImageInfos is a list of image.Info which can be filtered.
type ImageInfos []image.Info

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 2, 2018

Member

No-look suggestion: you could have two aliases here, to enforce where a list of images must be sorted in order to proceed. Might be more trouble than it's worth, though.

This comment has been minimized.

Copy link
@rndstr

rndstr Aug 2, 2018

Author Contributor

good suggestion. made an attempt!

@rndstr rndstr force-pushed the issue/706-semver-filtering-ordering branch 9 times, most recently from 440c710 to 3c7570f Aug 2, 2018
Copy link
Member

squaremo left a comment

This is a great PR, thanks Roli. Nits/suggestions, but if this all compiles/passes/works in practice, I'm happy with it.

@@ -142,7 +142,7 @@ func calculatePolicyChanges(opts *controllerPolicyOpts) (policy.Update, error) {
Add(policy.LockedUser)
}
if opts.tagAll != "" {
add = add.Set(policy.TagAll, policy.PatternAll.String())
add = add.Set(policy.TagAll, policy.NewPattern(opts.tagAll).String())

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 3, 2018

Member

ALL ALL ALL I see what you mean ...
This looks right (after some staring ..)

false: []string{"", "latest", "2.0.0"},
},
{
name: "semver",

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 3, 2018

Member

Different name?

}
}
filtered.Sort(pattern)
return filtered
}

func (ii ImageInfos) Sort(pattern policy.Pattern) {
image.Sort(ii, pattern.ImageNewerFunc())
image.Sort(ii, pattern.Newer)

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 3, 2018

Member

Yeah, that is a bit neater isn't it -- cool.

}
return filtered
// SortedImageInfos is a list of sorted image.Info
type SortedImageInfos []image.Info

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 3, 2018

Member

(and below ..) Oh cool, neat job. Could you make the fields of api/v6/Container SortedInfos, or does that screw things up?

rndstr added 2 commits Jul 27, 2018
Currently, containers can be tagged in manifests to filter what image
tags are considered when doing automated releases. Filtering is done by
specifying a wildcard glob. An optional prefix `glob:` can be used.

This PR adds support for tag filters based on [semantic versioning][0]
by using the prefix `semver:` instead. Version constraints can be
specified that filter images. Since versions have an implicit ordering
this also changes the way images are sorted when trying to determine the
newest image. For glob filtering this falls back to image creation date.

[0]: https://semver.org
@rndstr rndstr force-pushed the issue/706-semver-filtering-ordering branch from 3c7570f to 4c8fabb Aug 3, 2018
@rndstr

This comment has been minimized.

Copy link
Contributor Author

rndstr commented Aug 3, 2018

thanks for the thorough review @squaremo! I fixed up the commit history and will merge once #1263 makes it in as this one depends on it.

@rndstr rndstr changed the base branch from fix/test-mock-image-order to master Aug 6, 2018
@rndstr rndstr merged commit be0aa43 into master Aug 6, 2018
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@hiddeco hiddeco deleted the issue/706-semver-filtering-ordering branch Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.