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

Handle 1.x as a pattern correctly #153

Merged
merged 2 commits into from Jul 31, 2018

Conversation

Projects
None yet
3 participants
@philpennock
Contributor

philpennock commented Jul 2, 2018

The previous logic worked for 1.NN.x but not for 1.x. I had avoided use of regular expressions based on input, as an attack vector.

Validate the input to be sane, then use it to construct a regex after all, and search for proper releases.

Improve tests around .x handling to check for 1.x and to confirm not just that things resolve but that they resolve according to our documented assertion that we only issue a stable version number.

Fixes #152

@philpennock philpennock requested review from meatballhat and tianon Jul 2, 2018

philpennock added some commits Jul 2, 2018

Handle 1.x as a pattern correctly
The previous logic worked for `1.NN.x` but not for `1.x`.  I had avoided
use of regular expressions based on input, as an attack vector.

Validate the input to be sane, then use it to construct a regex after
all, and search for proper releases.

We need a way to stub out the updates and have tests for this
functionality.

Fixes #152
Improve tests around .x handling; silence debug trace
We had tests for `.x` resolution, but didn't test that the results were
our documented "stable versions" output.  Fix that.  Add a test for
`1.x` as a valid input.

Drop a debugging `warn` from the previous commit.
@philpennock

This comment has been minimized.

Contributor

philpennock commented Jul 2, 2018

Initial PR was accidentally including the code from PR 151, I rebased work against master. Sorry about that.

@dmitshur

This comment has been minimized.

dmitshur commented Jul 11, 2018

Issue is #152 is affecting me too, so I'm looking forward to a resolution. What is needed to make progress on this PR? /cc @meatballhat I'm happy to help if there's anything I can do. Thanks!

@dmitshur

This comment has been minimized.

dmitshur commented Jul 18, 2018

Friendly ping; my Travis build is still failing because of #152, so I'd really like to help fix it if there's anything I can do.

warn "resolve pattern '${base}.x' invalid for .x finding"
return 2
fi
# The `.x` is optional; "1.10" matches "1.10.x"

This comment has been minimized.

@tianon

tianon Jul 30, 2018

Collaborator

This seems kind of strange -- do we support 1.10.0 for users to get the actual upstream 1.10 release? (upstream doesn't do .0 releases, so while I agree that users would expect 1.10 to give them the same thing as 1.10.x, I don't think that's correct unless we add our own explicit support for a .0 suffix to mean "the first 1.10 release)

This comment has been minimized.

@philpennock

philpennock Jul 31, 2018

Contributor

The .x means "whatever goes here", where "whatever" might be "nothing".

We expect folks to be able to enter 1.42.x when 1.42 comes out so as to stay up-to-date on that release series. We have no other expected syntax to indicate this, since we don't have operators, only version numbers/patterns.

This comment has been minimized.

@tianon

tianon Jul 31, 2018

Collaborator

Ah, I get it now 👍

I had misread that as a user specifying 1.10 being the same as 1.10.x but it's instead that the latter will match the former.

LGTM

@philpennock philpennock merged commit b336d9d into master Jul 31, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@philpennock philpennock deleted the dot-x-in-minor branch Jul 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment