From 7de5d27dc07db88d1bf3d95f06ec51501f9d8a17 Mon Sep 17 00:00:00 2001 From: Paul Larsen Date: Thu, 3 Oct 2024 12:22:01 +0200 Subject: [PATCH] fix: Rework git tag semver resolution (#20083) (#20096) * Write initial tests Signed-off-by: Paul Larsen * Improve git tag semver resolution Signed-off-by: Paul Larsen * Add company to list of users Signed-off-by: Paul Larsen * Fix broken error string check Signed-off-by: Paul Larsen * Fix incorrect semver test assumption Signed-off-by: Paul Larsen * switch to debug statement Signed-off-by: Paul Larsen * Add more testcases for review Signed-off-by: Paul Larsen * review comments Signed-off-by: Paul Larsen --------- Signed-off-by: Paul Larsen --- USERS.md | 1 + util/git/client.go | 43 +++++++----- util/git/client_test.go | 142 ++++++++++++++++++++++++++++++++++++++++ util/git/git_test.go | 9 +-- 4 files changed, 171 insertions(+), 24 deletions(-) diff --git a/USERS.md b/USERS.md index 0452befb389ea..c7821e39265f6 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/) diff --git a/util/git/client.go b/util/git/client.go index 3b9c6d42450a5..80ec706062801 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. -// 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() } diff --git a/util/git/client_test.go b/util/git/client_test.go index c2f8061a1c2f0..207aebe9e34a0 100644 --- a/util/git/client_test.go +++ b/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) diff --git a/util/git/git_test.go b/util/git/git_test.go index bfa2905e7f518..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 @@ -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") } }) }