fix: Rework git tag semver resolution (#20083) #20096
Conversation
❗ Preview Environment deployment failed on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
✅ Preview Environment created on Bunnyshell but will not be auto-deployedSee: Environment Details Available commands (reply to this comment):
|
PaulSonOfLars
changed the title
PaulSonOfLars
force-pushed
the
paul/enforce-strict-semver
branch
from
e743b12
to
03f027b
Compare
|
@nitishfy apologies for that, the PR is just aiming to be a draft for now - to see if the silly basic approach would work against the full testsuite (onboarding and spinning up the tests locally is quite painful if you're not a regular contributor!). Clearly it doesn't! I've now narrowed down on the exact issue, and should be able to make the git semver logic match the helm logic, avoiding any big "breaking" changes. I'll aim to have that in a proper PR open with tests and the actual fix later tonight, ready for tomorrow's contributors meeting. |
PaulSonOfLars
force-pushed
the
paul/enforce-strict-semver
branch
from
03f027b
to
0a364c6
Compare
Signed-off-by: Paul Larsen <[email protected]>
Signed-off-by: Paul Larsen <[email protected]>
PaulSonOfLars
force-pushed
the
paul/enforce-strict-semver
branch
from
0a364c6
to
8ea5394
Compare
Signed-off-by: Paul Larsen <[email protected]>
Signed-off-by: Paul Larsen <[email protected]>
Signed-off-by: Paul Larsen <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20096 +/- ##
==========================================
+ Coverage 55.84% 55.86% +0.01%
==========================================
Files 321 321
Lines 44497 44501 +4
==========================================
+ Hits 24851 24860 +9
+ Misses 17075 17074 -1
+ Partials 2571 2567 -4 ☔ View full report in Codecov by Sentry. |
util/git/client.go
Outdated
| @@ -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()) | |||
There was a problem hiding this comment.
Should we make this a debug log?
There was a problem hiding this comment.
I agree with @rumstead's comment, otherwise LGTM. To unblock this error I'd ship this as-is, but I would like to see a follow-up PR which consolidates this with the Helm semver resolution.
Signed-off-by: Paul Larsen <[email protected]>
PaulSonOfLars
changed the title
| 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"], | ||
| }, { |
There was a problem hiding this comment.
According to the definition of a constraint in https://github.com/Masterminds/semver something like 4.1 can also be considered a constraint which can match any 4.1.x while at the same time it's also a valid semver.Version that translates to 4.1.0. This can still cause ambiguity, might require some proper documentation on this behaviour.
Testing a potential fix for #20083, where ArgoCD's semver resolution is too lax, which causes non-semver tags to be incorrectly coerced into semver. This then causes unexpected tags to be used when they shouldn't be.
Checklist: