fix(release): assure new release version is greater than the current one#376
fix(release): assure new release version is greater than the current one#376AlexxNica wants to merge 1 commit intoipfs:masterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
| if compare_version "$current_version" "$new_version"; then | ||
| echo "ERROR: The version provided ($new_version) has to be greater than the current release ($current_version)." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
This does not appear to catch the problem when:
current_version="2.3-rc1"
new_version="2.3" I get:
ERROR: The version provided (2.3) has to be greater than the current release (2.3-rc1).The version "2.3" should, semver-wise, be greater than "2.3-rc1".
Running a tool like semver-cli does catch this:
> semver-cli greater -v "2.3-rc1" "2.3"; echo "returned: $?"
2.3
returned: 1
> semver-cli greater -v "2.3" "2.3-rc1"; echo "returned: $?"
2.3
returned: 0Consider using semver-cli and changing the logic above to:
if ! semver-cli greater "$new_version" "$current_version"; then
echo "ERROR: The version provided ($new_version) has to be greater than the current release ($current_version)."
exit 1
fiThere was a problem hiding this comment.
@gammazero, that's because 2.3-rc1 is not compliant with the SemVer specification, as noted on Item 2 of the specification:
A normal version number MUST take the form X.Y.Z where X, Y, and Z are non-negative integers, and MUST NOT contain leading zeroes. X is the major version, Y is the minor version, and Z is the patch version. Each element MUST increase numerically. For instance: 1.9.0 -> 1.10.0 -> 1.11.0.
Thus, to be correctly formatted, the version must be separated by three dots.
There are some useful tips on the SemVer's FAQ that I use myself:
Q: Is there a suggested regular expression (RegEx) to check a SemVer string?
A: There are two. One with named groups for those systems that support them (PCRE [Perl Compatible Regular Expressions, i.e. Perl, PHP and R], Python and Go).
See: https://regex101.com/r/Ly7O1x/3/
^(?P<major>0|[1-9]\d*)\.(?P<minor>0|[1-9]\d*)\.(?P<patch>0|[1-9]\d*)(?:-(?P<prerelease>(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+(?P<buildmetadata>[0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$And one with numbered capture groups instead (so cg1 = major, cg2 = minor, cg3 = patch, cg4 = prerelease and cg5 = buildmetadata) that is compatible with ECMA Script (JavaScript), PCRE (Perl Compatible Regular Expressions, i.e. Perl, PHP and R), Python and Go.
See: https://regex101.com/r/vkijKf/1/
^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$
There was a problem hiding this comment.
Thank you for the detailed explanation. I took an example directly from the specification (item 11-3)
When major, minor, and patch are equal, a pre-release version has lower precedence than a normal version:
Example: 1.0.0-alpha < 1.0.0.
The code provided does not catch that "1.0.0-alpha" is less than "1.0.0"
There was a problem hiding this comment.
@gammazero, thank you for your review! You're right! And thanks for commenting on the StackOverflow answer related to this!
|
@AlexxNica: are you able to incorporate comments so we an get this merged? |
@BigLep absolutely! Will get that done ASAP. |
|
Awesome. Please set the status to "In Review" when it's ready to be looked at again. Thanks! |
|
@AlexxNica : just checking in. Are you able to incorporate comments? |
|
Sorry for the delay! I'd suggest we integrate any reliable package from npm that already handles everything SemVer-related and call it from the bash script to validate versions. I couldn't easily find a ready-to-use and reliable way of checking and comparing SemVer versions directly within bash, so migrating to a node script of some sort would be the best option unless we have the need to keep this logic within a bash script. |
|
@AlexxNica : I agree that it doesn't seem like a good idea to implementing this kind of thing in Bash - ugh. I dont' have experience with this repo, but if there's an npm package you have in mind for pulling in, feel free to do so. Thanks. |
|
@BigLep, Yea, implementing that in Bash is not a pretty thing.. 😰 As for the npm package, I'd use the battle-tested package used by node itself: https://github.com/npm/node-semver. I'll put this in my backlog and try getting it done within the next two weeks. |
|
Update: having to push this to probably the end of the month due to a project launch we'll have between this and next week. In the meantime, I've updated the PR description with a new checklist of to-dos. cc @BigLep |
After reviews and comments, we've decided to change the way of handling version checks. With that, here's an updated list of tasks to get this PR going:
Fixes #319