From f90dfa9bb2e505aed251bafacffd34023ee94ac6 Mon Sep 17 00:00:00 2001 From: Paul Larsen Date: Wed, 25 Sep 2024 17:25:15 +0100 Subject: [PATCH 1/8] Write initial tests Signed-off-by: Paul Larsen --- util/git/client_test.go | 126 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/util/git/client_test.go b/util/git/client_test.go index c2f8061a1c2f0..989c45fc82a89 100644 --- a/util/git/client_test.go +++ b/util/git/client_test.go @@ -173,6 +173,132 @@ 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.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"], + }, { + 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"], + }, { + // 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) From 8ea5394c6758534f50a7d53cbfa329f53b0c9d60 Mon Sep 17 00:00:00 2001 From: Paul Larsen Date: Wed, 25 Sep 2024 17:37:19 +0100 Subject: [PATCH 2/8] Improve git tag semver resolution Signed-off-by: Paul Larsen --- util/git/client.go | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/util/git/client.go b/util/git/client.go index 3b9c6d42450a5..6b5352b18c4bf 100644 --- a/util/git/client.go +++ b/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. +// 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.*"/"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 { +// * The revision is "v0.1.2"/"0.1.2", then this is not a constraint, it's a pinned version - so we fall back to the standard tag matching in the lsRemote loop. +// * The revision is "v0.1.*"/"0.1.*", and there's a tag matching that constraint, then we find the latest matching version and return its commit hash. +// * The revision is "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. +// * 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(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.Infof("Semver constraint '%s' resolved to tag '%s', at reference '%s'", revision, maxVersion.Original(), maxVersionHash.String()) return maxVersionHash.String() } From 6539140ca25839641355b21ebbf18498f973aa64 Mon Sep 17 00:00:00 2001 From: Paul Larsen Date: Wed, 25 Sep 2024 17:45:42 +0100 Subject: [PATCH 3/8] Add company to list of users Signed-off-by: Paul Larsen --- USERS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/USERS.md b/USERS.md index ab5dbc8c745c1..cbd9cc91d00dc 100644 --- a/USERS.md +++ b/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/) From cdf262f955b447e13f3243cf6a3eaddb3fa036e7 Mon Sep 17 00:00:00 2001 From: Paul Larsen Date: Wed, 25 Sep 2024 17:49:07 +0100 Subject: [PATCH 4/8] Fix broken error string check Signed-off-by: Paul Larsen --- util/git/git_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/git/git_test.go b/util/git/git_test.go index bfa2905e7f518..a58c9664b859c 100644 --- a/util/git/git_test.go +++ b/util/git/git_test.go @@ -299,7 +299,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") } }) } From ad99770e6f40a18a3344957db8e71c4446a48f69 Mon Sep 17 00:00:00 2001 From: Paul Larsen Date: Wed, 25 Sep 2024 17:50:42 +0100 Subject: [PATCH 5/8] Fix incorrect semver test assumption Signed-off-by: Paul Larsen --- util/git/git_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/util/git/git_test.go b/util/git/git_test.go index a58c9664b859c..d507c728c0fde 100644 --- a/util/git/git_test.go +++ b/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 From 1ab38821897950345d194abbaefcf63e7dc2a6f4 Mon Sep 17 00:00:00 2001 From: Paul Larsen Date: Sun, 29 Sep 2024 17:42:06 +0100 Subject: [PATCH 6/8] switch to debug statement Signed-off-by: Paul Larsen --- util/git/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/git/client.go b/util/git/client.go index 6b5352b18c4bf..9ba79ce252206 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -731,7 +731,7 @@ func (m *nativeGitClient) resolveSemverRevision(revision string, refs []*plumbin return "" } - log.Infof("Semver constraint '%s' resolved to tag '%s', at reference '%s'", revision, maxVersion.Original(), maxVersionHash.String()) + log.Debugf("Semver constraint '%s' resolved to tag '%s', at reference '%s'", revision, maxVersion.Original(), maxVersionHash.String()) return maxVersionHash.String() } From f25e6e6576afc147b87b04817d34d8bbae4ada31 Mon Sep 17 00:00:00 2001 From: Paul Larsen Date: Mon, 30 Sep 2024 17:13:21 +0100 Subject: [PATCH 7/8] Add more testcases for review Signed-off-by: Paul Larsen --- util/git/client_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/util/git/client_test.go b/util/git/client_test.go index 989c45fc82a89..207aebe9e34a0 100644 --- a/util/git/client_test.go +++ b/util/git/client_test.go @@ -187,6 +187,7 @@ func Test_SemverTags(t *testing.T) { "v1.0.0-rc1", "v1.0.0-rc2", "v1.0.0", + "v1.0", "v1.0.1", "v1.1.0", "2024-apple", @@ -242,6 +243,11 @@ func Test_SemverTags(t *testing.T) { 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", @@ -250,6 +256,16 @@ func Test_SemverTags(t *testing.T) { 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. From 2b0f039d33d1c37bce33d13934d467c189707dbe Mon Sep 17 00:00:00 2001 From: Paul Larsen Date: Wed, 2 Oct 2024 13:48:53 +0100 Subject: [PATCH 8/8] review comments Signed-off-by: Paul Larsen --- util/git/client.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/util/git/client.go b/util/git/client.go index 9ba79ce252206..80ec706062801 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -685,12 +685,12 @@ func (m *nativeGitClient) lsRemote(revision string) (string, error) { // resolveSemverRevision is a part of the lsRemote method workflow. // 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", then this is not a constraint, it's a pinned version - so we fall back to the standard tag matching in the lsRemote loop. -// * The revision is "v0.1.*"/"0.1.*", and there's a tag matching that constraint, then we find the latest matching version and return its commit hash. -// * The revision is "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. -// * 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; +// 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.