util/version: pre-release tags are not taken in to consideration in ShouldUpgrade()

[theckman]

In my organization we're working on using SemVer for all of our releases, with the use of pre-release tags to test experimental builds. For pre-release builds, we increment the patch and then add the pre-release information at the end. So if v1.2.2 is the last tag, the pre-release would be v1.2.3-preNNNNNN.

The problem is that keel does not currently inspect the information about the PreRelease to understand whether it's a "proper release"[1]. This means that if you have your policy set to patch, a pre-release of the next patch version will be deployed. To give a quick overview with each policy type there is a different combination of pre-release tags which someone may want to deploy:

• v1.2.2 => v1.2.3-pre
• v1.2.3 => v1.3.0-pre
• v1.2.3 => v2.0.0-pre
• v1.2.3-pre => v1.3.0-pre
• v1.2.3-pre => v1.3.0

Attached to the bottom of this issue is a patch with a unit test showing the failure mode that made me realize this was a potential issue. [2]

To solving the problem, it seems there are two ways to solve this problem within keel:

• Add more policies (e.g., PolicyTypePatchPreRelease, PolicyTypeMinorPreRelease, etc.) and do the additional checks
• Introduce a prelreasePolicy value that can contain the same values as policy does now (and maybe rename policy to releasePolicy). We could then have a matrix of behaviors based on the releasePolicy and the prereleasePolicy.

The second approach feels like the most powerful, and could allow you to do something like have a releasePolicy: minor so you get all of the minor + patch updates, but a prereleasePolicy: patch so that you only run experimental patch builds. This might be useful for something like a QA environment.

I think one of the easier ways to tackle this, since it's already a numerical value, to use it as bitwise flags. Policies could then be created by doing something like types.PolicyTypePreReleasePatch & types.PolicyTypeMinor. I'd be happy to take on implementing it once a good design is figured out.

[1] I quote proper release, because this could be up to who you ask.
[2]

diff --git a/util/version/version_test.go b/util/version/version_test.go
index f3c7d68..d172f8f 100644
--- a/util/version/version_test.go
+++ b/util/version/version_test.go
@@ -173,6 +173,16 @@ func TestShouldUpdate(t *testing.T) {
 			wantErr: false,
 		},
 		{
+			name: "patch pre-release, policy patch",
+			args: args{
+				current: &types.Version{Major: 1, Minor: 4, Patch: 5},
+				new:     &types.Version{Major: 1, Minor: 4, Patch: 6, PreRelease: "pre-20170728121212"},
+				policy:  types.PolicyTypePatch,
+			},
+			want:    false,
+			wantErr: false,
+		},
+		{
 			name: "patch increase, policy ptach",
 			args: args{
 				current: &types.Version{Major: 1, Minor: 4, Patch: 5},

[rusenask]

I think this problem also applies to all versions with metadata such release candidates (rc). Sometimes (might be quite often) you might not want to update to release candidate your production workloads.

We need a bit more generic solution that would still be easy to use and useful. How about new label that would have something like keel.sh/version=strict - skip all new versions that have metadata in the like pre release, release candidate.

Or it could be inverted, where you have to specify something to allow metadata in versions.

[theckman]

You are correct, it applies to any SemVer tag formatted like so: vN.N.N-PreReleaseInfo, where the PreReleaseInfo could be anything.

I like your idea of having something that's simple to use, and that label seems like it would handle most cases. I'm not sure I'm a huge fan of version with a value of strict because, at least in my mind, that's a bit ambiguous. If I saw that we were following strict versions, without knowing the context here, I'm not sure what I would assume the behavior is. I know this is absolutely bikeshedding, but how do you feel about something indicating it's a setting to do with pre-release tags (keel.sh/prerelaseTags=true/false) only because the SemVer spec refers to them as pre-release tags.

I think this solution would solve most deployment use cases, but for one of the use cases I had in mind with keel this solution wouldn't work for us. I can see situations, in keel, where I'd want the policy to be major but I'd only want to support deploying pre-release tags for patch-level increases. Here's a quick breakdown then of what versions it would deploy for us:

• v1.4.0 (yes)
• v1.4.1-rc1 (yes)
• v1.5.0 (yes)
• v1.5.0-rc1 (no)

Eventually I may want to relax the restrictions and have v1.5.0-rc1 get deployed too, but that would come after confidence is built in the SemVer processes + tooling we have. I think that's why I advocated for the more complex solution. Yes the additional complexity is there, but it gives consumers of the tool the power to really control the deployment flow without needing to write additional tooling or forking keel.

I was running out of energy when I opened this issue last night, and wasn't able to include a more concrete example of what I was thinking:

func stringToPolicyType(p string) types.PolicyType {
  // imagine this would take "minor" and return `types.PolicyTypeMinor`
}

func stringToPreReleasePolicy(p string) types.PolicyType {
  // imagine this would take "patch" and return `types.PolicyTypePreReleasePatch`
}

// assume cfg["policy"] lines up with the current policy setting,
// and that cfg["prerelease"] lines up with the pre-release values (same values as policy)
// cfg["policy"] = minor; cfg["prerelease"] = patch

// bit-wise AND to combine both conditions in to one integer
policy := stringToPolicyType(cfg["policy"]) & stringToPreReleasePolicy(cfg["prerelease"])

// we can then do bit-wise math to calculate the desired behaviors
// if our policy is a minor policy..
if policy&types.PolicyTypeMinor == types.PolicyTypeMinor {
  // if this is  a preRelease minor release, and our policy allows us to do minor pre-releases
  // Note: because of the policy we defined above, 
  // the second half of this expression should evaluate to false
  if isPreRelease(newVersion) && policy&types.PolicyTypePreReleaseMinor == types.PolicyTypePreReleaseMinor {
    // do the thing
  }
}

[rusenask]

Let's get back to this example with minor policy, needs a little bit reordering:

• 1.4.0 (yes)
• 1.4.1-rc1 (yes)
• 1.5.0-rc1 (no)
• 1.5.0 (yes)
• 1.5.1 (yes)

Could we just make this default behaviour? patch example:

• 1.5.0 (yes)
• 1.5.1-rc1 (no)
• 1.5.1 (yes)

Default behaviour - only allow minor & path pre-releases if policy is major.
And same for minor/patch versions (well, for patch no pre-releases would go through).

This would allow to safely upgrade to new versions. Policy all would allow any upgrades.

I think this would work for your use case.

[theckman]

In your first example, what if I want to test the 1.5.0-rc1, but wouldn't want an automatic upgrade to 2.0.0?

It's quite possible I'm not understanding what's being proposed, so let me try to make sure I understand. We'd have a flag that turns on support for managing pre-releases, and using that keel would make assumptions about how you want pre-release tags handled based on the policy you have selected.

• If this mode is enabled, keel will assume that for patch policy you only want to deploy patch changes and not patch-level pre-release tags.
• If you're at the minor policy, you will deploy released minor and patch versions, and pre-release patch versions.
• If you are at major, you will deploy released major, minor, and patch releases while deploying pre-release minor and patch tags?
• If you are using the all policy, you'd get all versions (pre-release and stable).

[azr]

Hey there ! I'm reviving this with another idea:

I believe everything needs to stay the same in the keel.sh configs.
When image tag has a pre-release tag then it's a special case and keel should update bumps to that special tag only.
Like it was another docker repo.

[rusenask]

I agree, I think that metadata is stored in either those fields (can't remember which one) https://github.com/keel-hq/keel/blob/master/types/types.go#L76-L77. It could be checked by k8s and helm providers. Need to check but maybe the only change has to be made in this function https://github.com/keel-hq/keel/blob/master/util/version/version.go#L119:6 .

[azr]

Trying something right now then 😄

[azr]

So that was quick, ShouldUpdate allowed updates without checking the pre-release tag, so I just needed to add a check 😄