Skip to content

Commit

Permalink
fix: Rework git tag semver resolution (#20083) (#20096)
Browse files Browse the repository at this point in the history 
* Write initial tests

Signed-off-by: Paul Larsen <pnvlarsen@gmail.com>

* Improve git tag semver resolution

Signed-off-by: Paul Larsen <pnvlarsen@gmail.com>

* Add company to list of users

Signed-off-by: Paul Larsen <pnvlarsen@gmail.com>

* Fix broken error string check

Signed-off-by: Paul Larsen <pnvlarsen@gmail.com>

* Fix incorrect semver test assumption

Signed-off-by: Paul Larsen <pnvlarsen@gmail.com>

* switch to debug statement

Signed-off-by: Paul Larsen <pnvlarsen@gmail.com>

* Add more testcases for review

Signed-off-by: Paul Larsen <pnvlarsen@gmail.com>

* review comments

Signed-off-by: Paul Larsen <pnvlarsen@gmail.com>

---------

Signed-off-by: Paul Larsen <pnvlarsen@gmail.com>
  • Loading branch information
@PaulSonOfLars
PaulSonOfLars authored Oct 3, 2024
1 parent 99560fb commit 7de5d27
Show file tree
Hide file tree
Showing 4 changed files with 171 additions and 24 deletions.
1 change: 1 addition & 0 deletions USERS.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/)
Expand Down
43 changes: 26 additions & 17 deletions util/git/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
}

Expand Down
142 changes: 142 additions & 0 deletions util/git/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
9 changes: 2 additions & 7 deletions util/git/git_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
}
})
}
Expand Down

0 comments on commit 7de5d27

Please sign in to comment.