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 the handling of pre-releases and the 0.0.0 release edge case #70

Merged
merged 1 commit into from
Apr 10, 2018

Conversation

mattfarina
Copy link
Member

0.0.0-alpha < 0.0.0-beta < 0.0.0 per the semver spec at
http://semver.org. The resolver ignores pre-release versions
unless the comparator has a pre-release. This change adds tests
for the cases around 0.0.0 along with fixing incorrect evaluations

0.0.0-alpha < 0.0.0-beta < 0.0.0 per the semver spec at
http://semver.org. The resolver ignores pre-release versions
unless the comparator has a pre-release. This change adds tests
for the cases around 0.0.0 along with fixing incorrect evaluations
Copy link

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattfarina I'm quite confused in general about this still, and I'm not confident on how the removed code were supposed to work and how it will work after it was removed. But all but one test made sense to me, see comment.

{">0", "0.0.1-alpha", false},
{">0.0", "0.0.1-alpha", false},
{">0-0", "0.0.1-alpha", true},
{">0.0-0", "0.0.1-alpha", true},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious to know if the following test cases also succeed. They should right?

{">0.0.0", "0.0.1-alpha", false},
{">0.0.0-0", "0.0.1-alpha", true},

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

{">=0.0-0", "0.0.1-alpha", true},
{">=0", "0.0.0-alpha", false},
{">=0-0", "0.0.0-alpha", true},
{"<0", "0.0.0-alpha", false},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confused about this test ({"<0", "0.0.0-alpha", false}), especially because {">=0", "0.0.0-alpha", false}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without a pre-release in the comparator (e.g., <0, >=0) you'll find all pre-release values are ignored. That's why they are false.

@mattfarina mattfarina merged commit 872a903 into master Apr 10, 2018
@mattfarina mattfarina deleted the fix/0-range branch April 10, 2018 20:06
david-yu pushed a commit to hashicorp/consul-helm that referenced this pull request Mar 19, 2021
Since Helm uses `Masterminds/semver` previous `kubeVersion` fields without any additional alphanumeric characters after the version would cause installs to fail on GKE and EKS. Since Masterminds/semver#70, we should now be able pass the pre-install check since ` 1.13.0-gke-version` < `1.13.0-0 `, `1.13.0-eks-version` < `1.13.0-0`, and `1.13.0` < `1.13.0-0`.
david-yu pushed a commit to hashicorp/consul-helm that referenced this pull request Mar 22, 2021
* Add back kubeVersion 

Since Helm uses `Masterminds/semver` previous `kubeVersion` fields without any additional alphanumeric characters after the version would cause installs to fail on GKE and EKS. Since Masterminds/semver#70, we should now be able pass the pre-install check since ` 1.13.0-gke-version` < `1.13.0-0 `, `1.13.0-eks-version` < `1.13.0-0`, and `1.13.0` < `1.13.0-0`.

* Adding changleog entry

* Adding ticks to changelog entry

* Temporarily enable nighly acc tests for this PR

* Revert "Temporarily enable nighly acc tests for this PR"

This reverts commit d99dd97.

* Update CHANGELOG.md

Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>

* Update CHANGELOG.md

Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>

Co-authored-by: Iryna Shustava <iryna@hashicorp.com>
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants