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 selecting incorrect latest version of Node #11116

Merged
merged 5 commits into from
Oct 15, 2019

Conversation

anthonychu
Copy link
Member

Currently there's a bug when selecting a node version. The task passes an array of available node versions to toolLib.evaluateVersions() that are prefixed with v (e.g. v12.8.0). This messes up the sorting inside the method, causing it to select the wrong version.

This is particularly noticeable when using a version spec of >=0.0.0 or * (both meaning select the absolute latest). It currently select 6.17.1 instead of the correct latest version of 12.8.0.

Changing the task so that it passes valid semantic versions (without v) to toolLib.evaluateVersions(), and returns the selected version with the leading v.

Fixing both V0 and V1.

/cc @sinedied @bnb

@bnb
Copy link

bnb commented Aug 13, 2019

Rubber stamp LGTM. Looked over the code and tests and this looks good - thank you for PRing Anthony!

@damccorm
Copy link

This generally looks good to me - could you bump the version in the task.json/task.loc.json files for both tasks?

@anthonychu
Copy link
Member Author

Whoops I missed this earlier. Bumped versions. Hopefully this is still relevant.

Copy link

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

LGTM

@damccorm damccorm merged commit 4eb8620 into microsoft:master Oct 15, 2019
leantk pushed a commit that referenced this pull request Dec 23, 2019
* Ignore 'v' prefix when evaluating latest in UseNodeV1

* Ignore 'v' prefix when evaluating latest in NodeToolV0

* Bump versions
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