-
Notifications
You must be signed in to change notification settings - Fork 32
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(release): assure new release version is greater than the current one #376
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There appears to be a problem with the logic when comparing rc with non-rc version.
Please be sure to test with all combinations of semver values.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: 0
Consider 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
fi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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