-
Notifications
You must be signed in to change notification settings - Fork 57
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 wrong Erlang/OTP version maint-x
matched when requesting x
#125
Fix wrong Erlang/OTP version maint-x
matched when requesting x
#125
Conversation
Don't take `maint-` -prefixed versions into account unless they're asked for it specifically At the same time, while fetching versions, make sure we identify them better so that 25 does not end up pulling maint-25 instead of OTP-25.0, as it should
@@ -6191,12 +6191,12 @@ async function maybeInstallRebar3(rebar3Spec) { | |||
|
|||
async function getOTPVersion(otpSpec0, osVersion) { | |||
const otpVersions = await getOTPVersions(osVersion) | |||
const otpSpec = otpSpec0.match(/^(OTP-)?([^ ]+)/) | |||
const otpSpec = otpSpec0.match(/^(OTP-|maint-)?([^ ]+)/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be case insensitive, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case do you expect this to change? I don't mind making it insensitive to case, but it's worked thus far...
let otpVersion | ||
if (otpSpec[1] && !isStrictVersion()) { | ||
throw new Error( | ||
`Requested Erlang/OTP version (${otpSpec0}) ` + | ||
"should not contain 'OTP-'", | ||
"should not contain 'OTP-, or maint-'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be sure I'm following along here, we're saying if the string you provided ends up matching two groups (OTP-
or maint-
) and version number, but you did not specify use strict versioning, we throw, is that all correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda, yes.
If the version string you (the consumer) provided:
- contains
OTP-
ormaint-
, and - is not set for strict versioning,
we throw.
What we're expecting is that a match can't go to a maint-
(in this case, adding to the previous case) unless asked for, explicitly.
I'm going ahead with merging this. We can maybe solve the details of your review in an upcoming release, if required. |
maint-x
matched when requesting x
maint-x
matched when requesting x
Don't take
maint-
-prefixed versions into account unless they're asked for it specifically.At the same time, while fetching versions, make sure we identify them better so that
25
does not end up pullingmaint-25
instead ofOTP-25.0
, as it should.Closes #120.