Add new "semver" constraint #6699

schmichael · 2019-11-13T23:46:25Z

The existing version constraint uses logic optimized for package
managers, not schedulers, when checking prereleases:

1.3.0-beta1 will not satisfy ">= 0.6.1"
1.7.0-rc1 will not satisfy ">= 1.6.0-beta1"

This is due to package managers wishing to favor final releases over
prereleases.

In a scheduler versions more often represent the earliest release all
required features/APIs are available in a system. Whether the constraint
or the version being evaluated are prereleases has no impact on
ordering.

This commit adds a new constraint - semver - which will use Semver
v2.0 ordering when evaluating constraints. Given the above examples:

1.3.0-beta1 satisfies ">= 0.6.1" using semver
1.7.0-rc1 satisfies ">= 1.6.0-beta1" using semver

Since existing jobspecs may rely on the old behavior, a new constraint
was added and the implicit Consul Connect and Vault constraints were
updated to use it.

dadgar · 2019-11-14T00:22:37Z

@schmichael Is it not just a bug in go-version? Doesn't this https://semver.org/#spec-item-11 state that precedence is defined left to right? So given our constraint is 0.6.1 and the current version is 0.13.1-beta1, you would see 6 < 13?

dadgar · 2019-11-14T00:47:44Z

This comment gives more details: hashicorp/go-version#36 (comment)

I guess for Terraform this makes... sense? I worry that the two constraints are so nearly identical it will be confusing to have both. Did you talk to James Bardin to see if removing the prerelease constraints is something they'd be willing to do or if it is a hard requirement for Terraform?

schmichael · 2019-11-14T16:32:30Z

I guess for Terraform this makes... sense?

Yes, that was the internal consensus. The current go-version behavior is modelled after npm where if you have a constraint of ">= 1.0" and the package is at "1.1-beta1", you'd rather download "1.0" than use a prerelease.

Nomad and other non-package-manager use cases just don't have the luxury of being able to download an artifact to satisfy missing constraints, so it's a different situation.

Likewise I completely dropped the ~> pessimistic operator because I feel like it only makes sense for package managers who are likely able to download the "right" version of a package if the constraint isn't immediately satisfied.

I worry that the two constraints are so nearly identical it will be confusing to have both.

Indeed. I just didn't see an alternative. Nomad doesn't even define the behavior of version in our docs: we instead point users to the go-version library directly. I can't imagine why someone would be relying on this behavior, but they may be relying on it without even knowing (eg some machines have prereleases of nvidia drivers that cause them to only get used when jobs specifically target that prerelease).

Did you talk to James Bardin to see if removing the prerelease constraints is something they'd be willing to do?

Yup, and Martin on the Terraform team. Martin wasn't too excited about me calling it "semver" because technically semver only covers ordering/precedence, and does not explicitly cover constraints semantics. I chose it anyway because (a) I couldn't think of another name that was less confusing and (b) reading the semver spec is sufficient to understand the operator's behavior.

helper/semvercon/semvercon.go

helper/semvercon/semvercon_test.go

scheduler/feasible.go

dadgar · 2019-11-14T21:55:13Z

scheduler/feasible.go

@@ -601,7 +606,7 @@ func checkLexicalOrder(op string, lVal, rVal interface{}) bool {

 // checkVersionMatch is used to compare a version on the
 // left hand side with a set of constraints on the right hand side
-func checkVersionMatch(ctx Context, lVal, rVal interface{}) bool {
+func checkVersionMatch(ctx Context, parse VerConstraintParser, lVal, rVal interface{}) bool {


Love how DRY you made this 🎉

scheduler/feasible.go

dadgar · 2019-11-18T16:49:38Z

@schmichael Looking really good. Feel free to ping when you want the final review (its still marked draft so I assume you have some more changes?)

The existing version constraint uses logic optimized for package managers, not schedulers, when checking prereleases: - 1.3.0-beta1 will *not* satisfy ">= 0.6.1" - 1.7.0-rc1 will *not* satisfy ">= 1.6.0-beta1" This is due to package managers wishing to favor final releases over prereleases. In a scheduler versions more often represent the earliest release all required features/APIs are available in a system. Whether the constraint or the version being evaluated are prereleases has no impact on ordering. This commit adds a new constraint - `semver` - which will use Semver v2.0 ordering when evaluating constraints. Given the above examples: - 1.3.0-beta1 satisfies ">= 0.6.1" using `semver` - 1.7.0-rc1 satisfies ">= 1.6.0-beta1" using `semver` Since existing jobspecs may rely on the old behavior, a new constraint was added and the implicit Consul Connect and Vault constraints were updated to use it.

schmichael · 2019-11-19T18:44:33Z

@dadgar Ready for final review. Rebased on master, squashed commits, added docs.

tgross

LGTM

helper/constraints/semver/constraints_test.go

schmichael · 2019-11-19T20:18:38Z

Going to merge to get the 0.10.2-rc1 train rolling, but there's still time to address any feedback you might have in a followup @dadgar

dadgar

LGTM

github-actions · 2023-01-25T02:16:22Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

dadgar requested changes Nov 14, 2019

View reviewed changes

schmichael force-pushed the f-semver-constraints branch from 3d4182c to 4e84a43 Compare November 18, 2019 21:40

schmichael added 2 commits November 19, 2019 08:40

vendor: update go-version to include NewSemver

1d2fa5a

schmichael force-pushed the f-semver-constraints branch from 4e84a43 to 75d6d4e Compare November 19, 2019 16:40

schmichael marked this pull request as ready for review November 19, 2019 16:48

schmichael self-assigned this Nov 19, 2019

schmichael added this to the 0.10.2 milestone Nov 19, 2019

schmichael requested a review from tgross November 19, 2019 16:49

docs: document semver constraint operator

88a9877

tgross approved these changes Nov 19, 2019

View reviewed changes

helper/constraints/semver/constraints_test.go Show resolved Hide resolved

test: assert semvers are *not* compared lexically

ecd4ed1

schmichael merged commit 6275132 into master Nov 19, 2019

schmichael deleted the f-semver-constraints branch November 19, 2019 20:18

dadgar reviewed Nov 20, 2019

View reviewed changes

github-actions bot locked as resolved and limited conversation to collaborators Jan 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new "semver" constraint #6699

Add new "semver" constraint #6699

schmichael commented Nov 13, 2019 •

edited

Loading

dadgar commented Nov 14, 2019

dadgar commented Nov 14, 2019 •

edited

Loading

schmichael commented Nov 14, 2019 •

edited

Loading

dadgar Nov 14, 2019

dadgar commented Nov 18, 2019 •

edited

Loading

schmichael commented Nov 19, 2019

tgross left a comment

schmichael commented Nov 19, 2019

dadgar left a comment

github-actions bot commented Jan 25, 2023

Add new "semver" constraint #6699

Add new "semver" constraint #6699

Conversation

schmichael commented Nov 13, 2019 • edited Loading

dadgar commented Nov 14, 2019

dadgar commented Nov 14, 2019 • edited Loading

schmichael commented Nov 14, 2019 • edited Loading

dadgar Nov 14, 2019

Choose a reason for hiding this comment

dadgar commented Nov 18, 2019 • edited Loading

schmichael commented Nov 19, 2019

tgross left a comment

Choose a reason for hiding this comment

schmichael commented Nov 19, 2019

dadgar left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 25, 2023

schmichael commented Nov 13, 2019 •

edited

Loading

dadgar commented Nov 14, 2019 •

edited

Loading

schmichael commented Nov 14, 2019 •

edited

Loading

dadgar commented Nov 18, 2019 •

edited

Loading