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 aliases #270

Merged
merged 18 commits into from Jun 30, 2021
Merged

Support LTS aliases #270

merged 18 commits into from Jun 30, 2021

Conversation

gordey4doronin
Copy link
Contributor

@gordey4doronin gordey4doronin commented Jun 17, 2021

The work is related to both #26 and #32.

For manifest changes see actions/versions-package-tools#32 and actions/node-versions#63.

  • Support lts/codename aliases
  • Support lts/* alias
  • Test coverage for lts/codename aliases
  • Test coverage for lts/* alias
  • Test coverage for malformed aliases
  • Clarify whether I need to commit compiled dist files or not? CC @maxim-lobanov
  • Discuss whether forcing check-latest = true is good idea or not? CC @maxim-lobanov
  • Discuss whether it's worth adding support for simple erbium or lts/erbium is enough? CC @maxim-lobanov @konradpabjan @bryanmacfarlane

The goal of my PR is to support .nvmrc syntax.
Given: .nvmrc/.node-version file in a repository
When: run setup-node action and pass content of the file to version property
Then: the action can recognize the format and choose correct node version

Being said that, from my point of view supporting simple erbium is not required, since version manager doesn't allow that.
Also, having all codenames starting with lts/ makes it really easy to determine in the code when lts alias passed to the action.


Reading the .nvmrc file is not the goal of this PR.
A separate PR shall be created for that, but it wouldn't be possible to implement .nvmrc reading without supporting the format.
So, think about current PR as a pre-requisite. 🙂

src/installer.ts Outdated Show resolved Hide resolved
src/installer.ts Outdated Show resolved Hide resolved
src/installer.ts Outdated Show resolved Hide resolved
src/installer.ts Outdated Show resolved Hide resolved
src/installer.ts Outdated Show resolved Hide resolved
@gordey4doronin gordey4doronin changed the title [WIP] Support LTS aliases Support LTS aliases Jun 22, 2021
@gordey4doronin gordey4doronin marked this pull request as ready for review June 22, 2021 14:06
src/installer.ts Outdated Show resolved Hide resolved
@maxim-lobanov
Copy link
Contributor

@gordey4doronin , could you please also add e2e test. See example: https://github.com/actions/setup-node/blob/main/.github/workflows/versions.yml#L32
I think adding new job lts-syntax with node-version: ["lts/*", "lts/Fermium"] should be enough.

P.S. I believe we also need to update docs but we will take care about it in separate PR since we will rework docs in scope of #272

@gordey4doronin
Copy link
Contributor Author

Screenshot 2021-06-23 at 09 35 15

It seems some mocks are lacking for windows test. Will fix later.

@gordey4doronin
Copy link
Contributor Author

could you please also add e2e test.

Absolutely. Good point. Will add too 👍

@gordey4doronin
Copy link
Contributor Author

  • Fixed unit tests for windows (was just a path resolution problem)
  • Added e2e tests for lts syntax
  • Unfortunately had to remove __tests__/verify-node.sh step from it, since the check script doesn't allow to parse lts syntax. However, I it should be enough to have only the main step, it's running actions/checkout@v2 and verifying lts alias resolution. If it couldn't resolve lts alias - it will fail. Profit.

@Kikobeats
Copy link

this is a super great functionality that has been not implemented until now, even this package is used for a lot of maintainers.

@maxim-lobanov @konradpabjan can you merge this, please? 🙏

__tests__/installer.test.ts Outdated Show resolved Hide resolved
src/installer.ts Outdated Show resolved Hide resolved
@maxim-lobanov maxim-lobanov merged commit bcb4cec into actions:main Jun 30, 2021
@gordey4doronin gordey4doronin deleted the gordey/support-lts-syntax branch June 30, 2021 08:32
@maxim-lobanov
Copy link
Contributor

@gordey4doronin @Kikobeats , Merged PR. I will cut new version later today. Need to merge one more PR before that.

@JimiC
Copy link

JimiC commented Jun 30, 2021

Good to see that it only took you about 2 years to finally implement "basic" functionality. Let's hope we, now, can use this action in our CI/CD.

P.S. Sorry if I sound cynical but 2 years is a loooong time for such a vital CI/CD component.

@skjnldsv
Copy link

Thank you everyone that worked on this! Great addition! 🎉 🤗 ❤️

@JimiC
Copy link

JimiC commented Jun 30, 2021

@skjnldsv What you don't know and that's why you reacted with 👎 is that I was the first to submit a PR with the exact same functionality 2 YEARS ago.

@skjnldsv
Copy link

@JimiC sorry, I understand your frustration, but I don't see how your message helps anyone nor benefit the community in any sort! 😕

Have a great day! ☀️

@JimiC
Copy link

JimiC commented Jun 30, 2021

@JimiC sorry, I understand your frustration, but I don't see how your message helps anyone nor benefit the community in any sort! 😕

@skjnldsv It's just that when GH actions went live I was so excited about it and one of the first to try it out. So much that I even tried to contribute to the community and the community turned their back on me. So they made me from a GH fan to a GitLab fan.

Have a great day! ☀️

You too. ☮️

@mrlubos
Copy link

mrlubos commented Jul 1, 2021

@gordey4doronin Can you help me to clarify my understanding? Can I at this point remove the node-version argument from the action setup and it will be able to infer the correct version from my .nvmrc file? If not, why? Would it be possible to add this functionality?

@gordey4doronin
Copy link
Contributor Author

@mrlubos Unfortunately you can't.👇 I wish that too. 🙏

Reading the .nvmrc file is not the goal of this PR.
A separate PR shall be created for that

"Resolving aliases" and "reading version manager file" are two different technical tasks.
They're related to each other from business PoV.
But they're not related from code/implementation PoV.

My goal in the first place was to add LTS aliases resolving.

Now it's possible to use this workaround if you have aliases in your .nvmrc file (which I do).

Reading the .nvmrc file is a standalone task, and was out of scope of my completely voluntarily PR.

For reading the file I personally up-voted #32 and actions/runner#1180. 🙂

Hope that makes sense.

@mrlubos
Copy link

mrlubos commented Jul 1, 2021

@mrlubos Unfortunately you can't.👇 I wish that too. 🙏

Reading the .nvmrc file is not the goal of this PR.
A separate PR shall be created for that

"Resolving aliases" and "reading version manager file" are two different technical tasks.
They're related to each other from business PoV.
But they're not related from code/implementation PoV.

My goal in the first place was to add LTS aliases resolving.

Now it's possible to use this workaround if you have aliases in your .nvmrc file (which I do).

Reading the .nvmrc file is a standalone task, and was out of scope of my completely voluntarily PR.

For reading the file I personally up-voted #32 and actions/runner#1180. 🙂

Hope that makes sense.

Thanks @gordey4doronin! I checked the issue you linked and what do you know, my vote is already there, too 😃 Have a nice day!

@jablko jablko mentioned this pull request May 2, 2022
2 tasks
deining pushed a commit to deining/setup-node that referenced this pull request Nov 9, 2023
Bumps [ts-node](https://github.com/TypeStrong/ts-node) from 10.2.0 to 10.2.1.
- [Release notes](https://github.com/TypeStrong/ts-node/releases)
- [Commits](TypeStrong/ts-node@v10.2.0...v10.2.1)

---
updated-dependencies:
- dependency-name: ts-node
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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

10 participants