From d8e3c0ee62fe49fc1f192c57974f5f18df8bc3fc Mon Sep 17 00:00:00 2001 From: Paul Larsen Date: Thu, 3 Oct 2024 18:15:25 +0100 Subject: [PATCH 1/3] cherrypick Signed-off-by: Paul Larsen --- USERS.md | 1 + util/git/client.go | 48 +++++++------- util/git/client_test.go | 142 ++++++++++++++++++++++++++++++++++++++++ util/git/git_test.go | 9 +-- 4 files changed, 170 insertions(+), 30 deletions(-) diff --git a/USERS.md b/USERS.md index 609129ee498dd..79af0d608c652 100644 --- a/USERS.md +++ b/USERS.md @@ -39,6 +39,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 3a2ea666fed7b..386d8c11106b1 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -630,11 +630,7 @@ func (m *nativeGitClient) lsRemote(revision string) (string, error) { revision = "HEAD" } - semverSha, err := m.resolveSemverRevision(revision, refs) - if err != nil { - return "", err - } - + semverSha := m.resolveSemverRevision(revision, refs) if semverSha != "" { return semverSha, nil } @@ -682,21 +678,29 @@ 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(revision string, refs []*plumbing.Reference) (string, error) { +// 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 { - return "", nil + log.Debugf("Revision '%s' is not a valid semver constraint, skipping semver resolution.", revision) + return "" } maxVersion := semver.New(0, 0, 0, "", "") @@ -709,12 +713,9 @@ func (m *nativeGitClient) resolveSemverRevision(revision string, refs []*plumbin tag := ref.Name().Short() version, err := semver.NewVersion(tag) if err != nil { - if errors.Is(err, semver.ErrInvalidSemVer) { - log.Debugf("Invalid semantic version: %s", tag) - continue - } - - return "", fmt.Errorf("error parsing version for tag: %w", err) + log.Debugf("Error parsing version for tag: '%s': %v", tag, err) + // Skip this tag and continue to the next one + continue } if constraint.Check(version) { @@ -726,10 +727,11 @@ func (m *nativeGitClient) resolveSemverRevision(revision string, refs []*plumbin } if maxVersionHash.IsZero() { - return "", nil + return "" } - return maxVersionHash.String(), nil + log.Debugf("Semver constraint '%s' resolved to tag '%s', at reference '%s'", revision, maxVersion.Original(), maxVersionHash.String()) + return maxVersionHash.String() } // CommitSHA returns current commit sha from `git rev-parse HEAD` diff --git a/util/git/client_test.go b/util/git/client_test.go index c5817fae34081..80c2bad4e2255 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 b471b4888b1ef..77c57cd61122e 100644 --- a/util/git/git_test.go +++ b/util/git/git_test.go @@ -232,15 +232,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 @@ -298,7 +293,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 1d5f86cb86bd4f431d7b8e3f1b9d01a2a32a0729 Mon Sep 17 00:00:00 2001 From: Paul Larsen Date: Fri, 4 Oct 2024 20:16:23 +0200 Subject: [PATCH 2/3] Update util/git/client_test.go Co-authored-by: Blake Pettersson Signed-off-by: Paul Larsen --- util/git/client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/git/client_test.go b/util/git/client_test.go index 80c2bad4e2255..2688b44792cea 100644 --- a/util/git/client_test.go +++ b/util/git/client_test.go @@ -176,7 +176,7 @@ func Test_ChangedFiles(t *testing.T) { func Test_SemverTags(t *testing.T) { tempDir := t.TempDir() - client, err := NewClientExt(fmt.Sprintf("file://%s", tempDir), tempDir, NopCreds{}, true, false, "", "") + client, err := NewClientExt(fmt.Sprintf("file://%s", tempDir), tempDir, NopCreds{}, true, false, "") require.NoError(t, err) err = client.Init() From d34afbe4ffe58c8b99bcb515bcfc675f2071e692 Mon Sep 17 00:00:00 2001 From: Paul Larsen Date: Mon, 7 Oct 2024 08:46:37 +0200 Subject: [PATCH 3/3] rm bad metrics cherry pick Signed-off-by: Paul Larsen --- applicationset/controllers/applicationset_controller_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go index 22049aeee01da..53ecf41bae677 100644 --- a/applicationset/controllers/applicationset_controller_test.go +++ b/applicationset/controllers/applicationset_controller_test.go @@ -6423,7 +6423,6 @@ func TestResourceStatusAreOrdered(t *testing.T) { argoObjs := []runtime.Object{} client := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(&cc.appSet).WithObjects(&cc.appSet).Build() - metrics := appsetmetrics.NewFakeAppsetMetrics(client) r := ApplicationSetReconciler{ Client: client, @@ -6433,7 +6432,6 @@ func TestResourceStatusAreOrdered(t *testing.T) { ArgoDB: &argoDBMock, ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), KubeClientset: kubeclientset, - Metrics: metrics, } err := r.updateResourcesStatus(context.TODO(), log.NewEntry(log.StandardLogger()), &cc.appSet, cc.apps)