fix: Rework git tag semver resolution (argoproj#20083) (argoproj#20096)

* Write initial tests

Signed-off-by: Paul Larsen <[email protected]>

* Improve git tag semver resolution

Signed-off-by: Paul Larsen <[email protected]>

* Add company to list of users

Signed-off-by: Paul Larsen <[email protected]>

* Fix broken error string check

Signed-off-by: Paul Larsen <[email protected]>

* Fix incorrect semver test assumption

Signed-off-by: Paul Larsen <[email protected]>

* switch to debug statement

Signed-off-by: Paul Larsen <[email protected]>

* Add more testcases for review

Signed-off-by: Paul Larsen <[email protected]>

* review comments

Signed-off-by: Paul Larsen <[email protected]>

---------

Signed-off-by: Paul Larsen <[email protected]>

Author: PaulSonOfLars

USERS.md

@@ -41,6 +41,7 @@ Currently, the following organizations are **officially** using Argo CD:
 1. [Beez Innovation Labs](https://www.beezlabs.com/)
 1. [Bedag Informatik AG](https://www.bedag.ch/)
 1. [Beleza Na Web](https://www.belezanaweb.com.br/)
+1. [Believable Bots](https://believablebots.io)
 1. [BigPanda](https://bigpanda.io)
 1. [BioBox Analytics](https://biobox.io)
 1. [BMW Group](https://www.bmwgroup.com/)

util/git/client.go

@@ -631,14 +631,9 @@ func (m *nativeGitClient) lsRemote(revision string) (string, error) {
 		revision = "HEAD"
 	}
 
-	// Check if the revision is a valid semver constraint before attempting to resolve it
-	if constraint, err := semver.NewConstraint(revision); err == nil {
-		semverSha := m.resolveSemverRevision(constraint, refs)
-		if semverSha != "" {
-			return semverSha, nil
-		}
-	} else {
-		log.Debugf("Revision '%s' is not a valid semver constraint, skipping semver resolution.", revision)
+	semverSha := m.resolveSemverRevision(revision, refs)
+	if semverSha != "" {
+		return semverSha, nil
 	}
 
 	// refToHash keeps a maps of remote refs to their hash
@@ -684,18 +679,31 @@ func (m *nativeGitClient) lsRemote(revision string) (string, error) {
 
 	// If we get here, revision string had non hexadecimal characters (indicating its a branch, tag,
 	// or symbolic ref) and we were unable to resolve it to a commit SHA.
-	return "", fmt.Errorf("Unable to resolve '%s' to a commit SHA", revision)
+	return "", fmt.Errorf("unable to resolve '%s' to a commit SHA", revision)
 }
 
 // resolveSemverRevision is a part of the lsRemote method workflow.
-// When the user configure correctly the Git repository revision and the revision is a valid semver constraint
-// only the for loop in this function will run, otherwise the lsRemote loop will try to resolve the revision.
-// Some examples to illustrate the actual behavior, if:
-// * The revision is "v0.1.*"/"0.1.*" or "v0.1.2"/"0.1.2" and there's a tag matching that constraint only this function loop will run;
-// * The revision is "v0.1.*"/"0.1.*" or "0.1.2"/"0.1.2" and there is no tag matching that constraint this function loop and lsRemote loop will run for backward compatibility;
-// * The revision is "custom-tag" only the lsRemote loop will run because that revision is an invalid semver;
-// * The revision is "master-branch" only the lsRemote loop will run because that revision is an invalid semver;
-func (m *nativeGitClient) resolveSemverRevision(constraint *semver.Constraints, refs []*plumbing.Reference) string {
+// When the user correctly configures the Git repository revision, and that revision is a valid semver constraint, we
+// use this logic path rather than the standard lsRemote revision resolution loop.
+// Some examples to illustrate the actual behavior - if the revision is:
+// * "v0.1.2"/"0.1.2" or "v0.1"/"0.1", then this is not a constraint, it's a pinned version - so we fall back to the standard tag matching in the lsRemote loop.
+// * "v0.1.*"/"0.1.*", and there's a tag matching that constraint, then we find the latest matching version and return its commit hash.
+// * "v0.1.*"/"0.1.*", and there is *no* tag matching that constraint, then we fall back to the standard tag matching in the lsRemote loop.
+// * "custom-tag", only the lsRemote loop will run - because that revision is an invalid semver;
+// * "master-branch", only the lsRemote loop will run because that revision is an invalid semver;
+func (m *nativeGitClient) resolveSemverRevision(revision string, refs []*plumbing.Reference) string {
+	if _, err := semver.NewVersion(revision); err == nil {
+		// If the revision is a valid version, then we know it isn't a constraint; it's just a pin.
+		// In which case, we should use standard tag resolution mechanisms.
+		return ""
+	}
+
+	constraint, err := semver.NewConstraint(revision)
+	if err != nil {
+		log.Debugf("Revision '%s' is not a valid semver constraint, skipping semver resolution.", revision)
+		return ""
+	}
+
 	maxVersion := semver.New(0, 0, 0, "", "")
 	maxVersionHash := plumbing.ZeroHash
 	for _, ref := range refs {
@@ -723,6 +731,7 @@ func (m *nativeGitClient) resolveSemverRevision(constraint *semver.Constraints,
 		return ""
 	}
 
+	log.Debugf("Semver constraint '%s' resolved to tag '%s', at reference '%s'", revision, maxVersion.Original(), maxVersionHash.String())
 	return maxVersionHash.String()
 }
 

util/git/client_test.go

@@ -173,6 +173,148 @@ func Test_ChangedFiles(t *testing.T) {
 	assert.ElementsMatch(t, []string{"README"}, changedFiles)
 }
 
+func Test_SemverTags(t *testing.T) {
+	tempDir := t.TempDir()
+
+	client, err := NewClientExt(fmt.Sprintf("file://%s", tempDir), tempDir, NopCreds{}, true, false, "", "")
+	require.NoError(t, err)
+
+	err = client.Init()
+	require.NoError(t, err)
+
+	mapTagRefs := map[string]string{}
+	for _, tag := range []string{
+		"v1.0.0-rc1",
+		"v1.0.0-rc2",
+		"v1.0.0",
+		"v1.0",
+		"v1.0.1",
+		"v1.1.0",
+		"2024-apple",
+		"2024-banana",
+	} {
+		err = runCmd(client.Root(), "git", "commit", "-m", tag+" commit", "--allow-empty")
+		require.NoError(t, err)
+
+		// Create an rc semver tag
+		err = runCmd(client.Root(), "git", "tag", tag)
+		require.NoError(t, err)
+
+		sha, err := client.LsRemote("HEAD")
+		require.NoError(t, err)
+
+		mapTagRefs[tag] = sha
+	}
+
+	for _, tc := range []struct {
+		name     string
+		ref      string
+		expected string
+		error    bool
+	}{{
+		name:     "pinned rc version",
+		ref:      "v1.0.0-rc1",
+		expected: mapTagRefs["v1.0.0-rc1"],
+	}, {
+		name:     "lt rc constraint",
+		ref:      "< v1.0.0-rc3",
+		expected: mapTagRefs["v1.0.0-rc2"],
+	}, {
+		name:     "pinned major version",
+		ref:      "v1.0.0",
+		expected: mapTagRefs["v1.0.0"],
+	}, {
+		name:     "pinned patch version",
+		ref:      "v1.0.1",
+		expected: mapTagRefs["v1.0.1"],
+	}, {
+		name:     "pinned minor version",
+		ref:      "v1.1.0",
+		expected: mapTagRefs["v1.1.0"],
+	}, {
+		name:     "patch wildcard constraint",
+		ref:      "v1.0.*",
+		expected: mapTagRefs["v1.0.1"],
+	}, {
+		name:     "patch tilde constraint",
+		ref:      "~v1.0.0",
+		expected: mapTagRefs["v1.0.1"],
+	}, {
+		name:     "minor wildcard constraint",
+		ref:      "v1.*",
+		expected: mapTagRefs["v1.1.0"],
+	}, {
+		// The semver library allows for using both * and x as the wildcard modifier.
+		name:     "alternative minor wildcard constraint",
+		ref:      "v1.x",
+		expected: mapTagRefs["v1.1.0"],
+	}, {
+		name:     "minor gte constraint",
+		ref:      ">= v1.0.0",
+		expected: mapTagRefs["v1.1.0"],
+	}, {
+		name:     "multiple constraints",
+		ref:      "> v1.0.0 < v1.1.0",
+		expected: mapTagRefs["v1.0.1"],
+	}, {
+		// We treat non-specific semver versions as regular tags, rather than constraints.
+		name:     "non-specific version",
+		ref:      "v1.0",
+		expected: mapTagRefs["v1.0"],
+	}, {
+		// Which means a missing tag will raise an error.
+		name:  "missing non-specific version",
+		ref:   "v1.1",
+		error: true,
+	}, {
+		// This is NOT a semver constraint, so it should always resolve to itself - because specifying a tag should
+		// return the commit for that tag.
+		// semver/v3 has the unfortunate semver-ish behaviour where any tag starting with a number is considered to be
+		// "semver-ish", where that number is the semver major version, and the rest then gets coerced into a beta
+		// version string. This can cause unexpected behaviour with constraints logic.
+		// In this case, if the tag is being incorrectly coerced into semver (for being semver-ish), it will incorrectly
+		// return the commit for the 2024-banana tag; which we want to avoid.
+		name:     "apple non-semver tag",
+		ref:      "2024-apple",
+		expected: mapTagRefs["2024-apple"],
+	}, {
+		name:     "banana non-semver tag",
+		ref:      "2024-banana",
+		expected: mapTagRefs["2024-banana"],
+	}, {
+		// A semver version (without constraints) should ONLY match itself.
+		// We do not want "2024-apple" to get "semver-ish'ed" into matching "2024.0.0-apple"; they're different tags.
+		name:  "no semver tag coercion",
+		ref:   "2024.0.0-apple",
+		error: true,
+	}, {
+		// No minor versions are specified, so we would expect a major version of 2025 or more.
+		// This is because if we specify > 11 in semver, we would not expect 11.1.0 to pass; it should be 12.0.0 or more.
+		// Similarly, if we were to specify > 11.0, we would expect 11.1.0 or more.
+		name:  "semver constraints on non-semver tags",
+		ref:   "> 2024-apple",
+		error: true,
+	}, {
+		// However, if one specifies the minor/patch versions, semver constraints can be used to match non-semver tags.
+		// 2024-banana is considered as "2024.0.0-banana" in semver-ish, and banana > apple, so it's a match.
+		// Note: this is more for documentation and future reference than real testing, as it seems like quite odd behaviour.
+		name:     "semver constraints on non-semver tags",
+		ref:      "> 2024.0.0-apple",
+		expected: mapTagRefs["2024-banana"],
+	}} {
+		t.Run(tc.name, func(t *testing.T) {
+			commitSHA, err := client.LsRemote(tc.ref)
+			if tc.error {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			assert.True(t, IsCommitSHA(commitSHA))
+			assert.Equal(t, tc.expected, commitSHA)
+		})
+	}
+}
+
 func Test_nativeGitClient_Submodule(t *testing.T) {
 	tempDir, err := os.MkdirTemp("", "")
 	require.NoError(t, err)

util/git/git_test.go

@@ -233,15 +233,10 @@ func TestLsRemote(t *testing.T) {
 			expectedCommit: "ff87d8cb9e669d3738434733ecba3c6dd2c64d70",
 		},
 		{
-			name:           "should resolve a pined tag with semantic versioning",
+			name:           "should resolve a pinned tag with semantic versioning",
 			revision:       "v0.8.0",
 			expectedCommit: "d7c04ae24c16f8ec611b0331596fbc595537abe9",
 		},
-		{
-			name:           "should resolve a pined tag with semantic versioning without the 'v' prefix",
-			revision:       "0.8.0",
-			expectedCommit: "d7c04ae24c16f8ec611b0331596fbc595537abe9",
-		},
 		{
 			name:           "should resolve a range tag with semantic versioning",
 			revision:       "v0.8.*", // it should resolve to v0.8.2
@@ -299,7 +294,7 @@ func TestLsRemote(t *testing.T) {
 
 		for _, revision := range xfail {
 			_, err := clnt.LsRemote(revision)
-			assert.ErrorContains(t, err, "Unable to resolve")
+			assert.ErrorContains(t, err, "unable to resolve")
 		}
 	})
 }