build: INFENG-926: Fix version.sh version string output #10085
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes a bug in version.sh that would cause an invalid local version string to be output under certain circumstances; specifically, if a tag were made on a commit that was not on the current branch (i.e. reachable with git-describe). This took a code path in version.sh that did not properly remove any existing tag +metadata with grep, leading to version strings like 0.751.0+dryrun+27a014b44. Note the extra plus sign (+). This causes downstream tools, like helm, to fail. Now, we remove any existing version metadata from the final for all code paths.
✅ Deploy Preview for determined-ui canceled.
|
(https://circleci.com/docs/configuration-reference/#checkout), I believe we need to add a git-fetch after checkout to grab additional refs necessary for version.sh to work properly on CI.
Use explicit [0-9] character classes, because apparently BSD grep's -E mode includes \d classes, while GNU grep's -E does not. This was causing the script to run correctly on macOS, but not on Linux. Delightful.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10085 +/- ##
=======================================
Coverage 54.39% 54.39%
=======================================
Files 1267 1267
Lines 159323 159323
Branches 3632 3630 -2
=======================================
Hits 86670 86670
Misses 72519 72519
Partials 134 134
Flags with carried forward coverage won't be shown. Click here to find out more. |
Remove git fetch --tags and debug version.sh invocation from .circleci/config.yml.
molinamelendezj
approved these changes
Oct 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
INFENG-926
Description
This fixes a bug in version.sh that would cause an invalid local version string to be output under certain circumstances; specifically, if a tag were made on a commit that was not on the current branch (i.e. reachable with git-describe). This took a code path in version.sh that did not properly remove any existing tag +metadata with grep, leading to version strings like 0.751.0+dryrun+27a014b44. Note the extra plus sign (+). This causes downstream tools, like helm, to fail.
Now, we remove any existing version metadata from the final for all code paths.
This also changes all instances of
\dcharacter classes to[0-9]. This fixes a cross-platform issue where BSD grep's-Eincludes\dclasses, while GNU grep's-Edoes not. This means the script worked fine on macOS, but broke subtly in CI (and I suppose on anyone's work machine that uses Linux). We avoid this entirely by falling back to standard character sets.Test Plan
+in it on a commit on a branch other than this one,INFENG-926-fix-version-sh. E.g.v0.800.0+dryrun.INFENG-926-fix-version-shand run./version.sh. The version string returned should have only one+metadatapart, instead of two.Checklist
docs/release-notes/See Release Note for details.