Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: cherrypick semver fix #20096 into 2.12 #20215

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions USERS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/)
Expand Down
48 changes: 25 additions & 23 deletions util/git/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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, "", "")
Expand All @@ -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) {
Expand All @@ -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`
Expand Down
142 changes: 142 additions & 0 deletions util/git/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,148 @@
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, "", "")

Check failure on line 179 in util/git/client_test.go

View workflow job for this annotation

GitHub Actions / Lint Go code

cannot use "" (untyped string constant) as ClientOpts value in argument to NewClientExt (typecheck)

Check failure on line 179 in util/git/client_test.go

View workflow job for this annotation

GitHub Actions / Run unit tests for Go packages

cannot use "" (untyped string constant) as ClientOpts value in argument to NewClientExt

Check failure on line 179 in util/git/client_test.go

View workflow job for this annotation

GitHub Actions / Run unit tests for Go packages

cannot use "" (untyped string constant) as ClientOpts value in argument to NewClientExt

Check failure on line 179 in util/git/client_test.go

View workflow job for this annotation

GitHub Actions / Run unit tests with -race for Go packages

cannot use "" (untyped string constant) as ClientOpts value in argument to NewClientExt

Check failure on line 179 in util/git/client_test.go

View workflow job for this annotation

GitHub Actions / Run unit tests with -race for Go packages

cannot use "" (untyped string constant) as ClientOpts value in argument to NewClientExt
PaulSonOfLars marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down
9 changes: 2 additions & 7 deletions util/git/git_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
}
})
}
Expand Down
Loading