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

Prerelease constraint handling #36

Closed
nevans opened this issue May 10, 2018 · 5 comments
Closed

Prerelease constraint handling #36

nevans opened this issue May 10, 2018 · 5 comments

Comments

@nevans
Copy link

nevans commented May 10, 2018

The logic in fbe76cf doesn't seem quite right. In particular, consider the use-case here: https://github.com/gesellix/couchdb-prometheus-exporter/blob/master/lib/couchdb-client.go#L52-L69 where we are checking if the server version is >= 2.0 and the reported version is e.g. 2.2.0-302bd8b. Clearly, in this scenario, the constraint check should return true (but it returns false). At the very least, the constraint needs to be able to opt-out of this mode.

https://semver.org/#spec-item-11 implies that 2.2.0-pre >= 2.0 should be true:
"1.0.0-alpha < 1.0.0-alpha.1 < 1.0.0-alpha.beta < 1.0.0-beta < 1.0.0-beta.2 < 1.0.0-beta.11 < 1.0.0-rc.1 < 1.0.0." I can certainly see the case for not wanting to match pre-releases at all in certain circumstances.

@nevans
Copy link
Author

nevans commented May 10, 2018

As a test case:

package main

import (
        "log"

        "github.com/hashicorp/go-version"
)

func main() {
        constraint, _ := version.NewConstraint(">= 2.0")
        version, _ := version.NewVersion("2.2.0-302bd8b")
        ok := constraint.Check(version)
        log.Printf("version check: %#v", ok)
}
 $  go run ./test-version.go
2018/05/10 13:54:22 version check: false

@WingGao
Copy link

WingGao commented Jun 21, 2018

Same problem.

@mitchellh
Copy link
Contributor

@jbardin You're probably more intimately familiar with this, do you mind taking a look?

@jbardin
Copy link
Member

jbardin commented Jul 16, 2018

Hi @nevans,

I think the comparison is correctly happening according to the specification. Semantic versioning alone doesn't define the constraint operations, only the comparisons between versions. If you take the examples versions you provided and compare them, you get

major := version.Must(version.NewVersion("2.0"))
pre := version.Must(version.NewVersion("2.2.0-302bd8b"))
fmt.Printf("%s > %s = %t\n", pre, major, pre.GreaterThan(major))
// 2.2.0-302bd8b > 2.0.0 = true

Which follows Item 11 in the SemVer spec.

The difference from your first example is that when a user adds ">= 2.0" as a versions constraint, it implies "any released version greater than or equal to 2.0". Looking at NPM, which has another published specification with the similar constraint operators you can see in the Prerelease tags section:

If a version has a prerelease tag (for example, 1.2.3-alpha.3) then it will only be allowed to satisfy comparator sets if at least one comparator with the same [major, minor, patch] tuple also has a prerelease tag

The proper constraint to include 2.2.0 alphanumeric preleases like this 2.2.0-302bd8b would be >=2.2.0-0.

There is however still a point of contention here. As shown above, NPM would require (Major, Minor, Patch) to match a pre-release, which is the behavior we followed in this package. I did mention Ruby Gems though, and we differ there as they chose to match any (Major Minor, Patch) as long as a prerelease was specified. For example

> Gem::Dependency.new('', '>= 2.0').match?('', '2.2.0-302bd8b')
=> false
> Gem::Dependency.new('', '>= 2.0.0-0').match?('', '2.2.0-302bd8b')
=> true

So Ruby would allow matching between the versions you specified, provided you start with a more precise constraint string.

Now we did choose to follow the former behavior rather than the latter, since it provided a safer constraint set without compound statements being provided. Hopefully this clears up the situation, and we can better document this behavior for users.

@nevans
Copy link
Author

nevans commented Aug 8, 2018

Thanks, @jbardin for your explanation. I can see why versioning constraints for selecting compatible libraries would want these constraints for prereleases, even though both Ruby gems and NPM fail the principle of least surprise for me. 😉 I'd expected the >= to follow strict comparison ordering.

For our use case, we needed feature detection on a running server, which means that pre-releases shouldn't be treated as special. At any rate, as per your example, we could use a simple GreaterThan comparison without issue. I hadn't thought of that. Since our use case was very simple, we simply handled it by replacing go-version with a strings.Split, strconv.Atoi, and major >= 2. https://github.com/nevans/couchdb-prometheus-exporter/blob/df807430dc1007376f189581a8b1f9d51e58e18d/lib/couchdb-client.go#L53-L66

Thanks for taking the time to explain it.

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

No branches or pull requests

4 participants