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

Support lts/-n aliases #481

Merged
merged 5 commits into from Jun 2, 2022
Merged

Support lts/-n aliases #481

merged 5 commits into from Jun 2, 2022

Conversation

jablko
Copy link
Contributor

@jablko jablko commented May 2, 2022

Description:
#270 added support for lts/<alias> and lts/* nvm aliases. This adds support for nvm's (and hence .nvmrc/node-version-file's) lts/-n: nvm-sh/nvm#1998 (comment)

Related issue:
#481

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@jablko jablko requested a review from a team as a code owner May 2, 2022
@e-korolevskii
Copy link
Contributor

e-korolevskii commented May 6, 2022

Hi, @jablko,

You need to manually run build and add the results to the PR.

Cheers.

@jablko
Copy link
Contributor Author

jablko commented May 6, 2022

  • Thank you @e-korolevskii! I've added the build products to the commit.

@dmitry-shibanov
Copy link
Contributor

dmitry-shibanov commented May 10, 2022

Hello @jablko. Could you please add e2e tests for new syntax ?

@jablko
Copy link
Contributor Author

jablko commented May 10, 2022

@jablko jablko force-pushed the patch-1 branch 2 times, most recently from 2358e7c to 7ababba Compare May 10, 2022
@jablko
Copy link
Contributor Author

jablko commented May 10, 2022

@dmitry-shibanov I noticed:

  1. The e2e test exposed a mistake I made and have now corrected.
  2. The lts-syntax e2e test was missing a "Verify node and npm" step like the other versions.yml jobs, so I added that too.

@jablko
Copy link
Contributor Author

jablko commented May 13, 2022

I've rebased and combined the repetitive LTS tests with it.each(table) --- but I'm happy to leave them unrolled if you prefer?

@dmitry-shibanov
Copy link
Contributor

dmitry-shibanov commented May 30, 2022

Hello @jablko. Could you please sync with the main branch ?

@jablko
Copy link
Contributor Author

jablko commented May 30, 2022

@dmitry-shibanov
Copy link
Contributor

dmitry-shibanov commented May 30, 2022

Hello @jablko. I think we should not reverse initial manifest. If we specify check-latest to true the action won't return the latest lts version for lts aliases.

The code return the major version that is why when searching for a satisfied version in the reversed array, not the latest version will be taken.

@jablko
Copy link
Contributor Author

jablko commented May 30, 2022

Thanks @dmitry-shibanov, you're right, tc.findFromManifest() depends on the manifest's order.

  • I've added an entry to the unit tests' mock manifest, so one of its major versions has at least two minor versions, which exposes my mistake. Then I removed the .reverse() and confirmed the tests pass again and we get the max satisfying version when check-latest is true.

Copy link

@callree callree left a comment

CI

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.

None yet

6 participants