Skip to content
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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlexxNica
Copy link

@AlexxNica AlexxNica commented Jul 30, 2021

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

@welcome

This comment has been minimized.

Copy link
Contributor

@gammazero gammazero left a 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
Copy link
Contributor

@gammazero gammazero Jul 30, 2021

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

Copy link
Author

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-]+)*))?$

Copy link
Contributor

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"

Copy link
Author

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!

@BigLep
Copy link
Contributor

BigLep commented Mar 10, 2022

@AlexxNica: are you able to incorporate comments so we an get this merged?

@BigLep BigLep added the need/author-input Needs input from the original author label Mar 10, 2022
@AlexxNica
Copy link
Author

@AlexxNica: are you able to incorporate comments so we an get this merged?

@BigLep absolutely! Will get that done ASAP.

@BigLep BigLep marked this pull request as draft March 11, 2022 05:06
@BigLep BigLep marked this pull request as ready for review March 11, 2022 05:09
@BigLep
Copy link
Contributor

BigLep commented Mar 11, 2022

Awesome. Please set the status to "In Review" when it's ready to be looked at again. Thanks!

@BigLep
Copy link
Contributor

BigLep commented Mar 25, 2022

@AlexxNica : just checking in. Are you able to incorporate comments?

@lidel lidel marked this pull request as draft April 25, 2022 18:03
@lidel lidel removed the need/author-input Needs input from the original author label Apr 25, 2022
@AlexxNica
Copy link
Author

@BigLep

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.

@BigLep
Copy link
Contributor

BigLep commented Apr 27, 2022

@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.

@AlexxNica
Copy link
Author

@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.

@AlexxNica
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: 🥞 Todo
Development

Successfully merging this pull request may close these issues.

dist.sh should check that the new version is newer than the current version
4 participants