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鈥檒l 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 Jun 22, 2021
src/installer.ts Outdated Show resolved Hide resolved
@maxim-lobanov
Copy link
Collaborator

@maxim-lobanov maxim-lobanov commented Jun 23, 2021

@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

@gordey4doronin gordey4doronin commented Jun 23, 2021

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

@gordey4doronin gordey4doronin commented Jun 23, 2021

could you please also add e2e test.

Absolutely. Good point. Will add too 馃憤

@gordey4doronin
Copy link
Contributor Author

@gordey4doronin gordey4doronin commented Jun 25, 2021

  • 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

@Kikobeats Kikobeats commented Jun 28, 2021

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
58 checks passed
58 checks passed
@github-actions
Check licenses
Details
@github-actions
build (ubuntu-latest)
Details
@github-actions
test-proxy
Details
@github-actions
local-cache (ubuntu-latest, 10)
Details
@github-actions
build (windows-latest)
Details
@github-actions
local-cache (ubuntu-latest, 12)
Details
@github-actions
build (macos-latest)
Details
@github-actions
local-cache (ubuntu-latest, 14)
Details
@github-actions
local-cache (windows-latest, 10)
Details
@github-actions
local-cache (windows-latest, 12)
Details
@github-actions
local-cache (windows-latest, 14)
Details
@github-actions
local-cache (macos-latest, 10)
Details
@github-actions
local-cache (macos-latest, 12)
Details
@github-actions
local-cache (macos-latest, 14)
Details
@github-actions
test-bypass-proxy
Details
@github-actions
lts-syntax (ubuntu-latest, lts/dubnium)
Details
@github-actions
lts-syntax (ubuntu-latest, lts/erbium)
Details
@github-actions
lts-syntax (ubuntu-latest, lts/fermium)
Details
@github-actions
lts-syntax (ubuntu-latest, lts/*)
Details
@github-actions
lts-syntax (windows-latest, lts/dubnium)
Details
@github-actions
lts-syntax (windows-latest, lts/erbium)
Details
@github-actions
lts-syntax (windows-latest, lts/fermium)
Details
@github-actions
lts-syntax (windows-latest, lts/*)
Details
@github-actions
lts-syntax (macos-latest, lts/dubnium)
Details
@github-actions
lts-syntax (macos-latest, lts/erbium)
Details
@github-actions
lts-syntax (macos-latest, lts/fermium)
Details
@github-actions
lts-syntax (macos-latest, lts/*)
Details
@github-actions
manifest (ubuntu-latest, 10.15)
Details
@github-actions
manifest (ubuntu-latest, 12.16.0)
Details
@github-actions
manifest (ubuntu-latest, 14.2.0)
Details
@github-actions
manifest (windows-latest, 10.15)
Details
@github-actions
manifest (windows-latest, 12.16.0)
Details
@github-actions
manifest (windows-latest, 14.2.0)
Details
@github-actions
manifest (macos-latest, 10.15)
Details
@github-actions
manifest (macos-latest, 12.16.0)
Details
@github-actions
manifest (macos-latest, 14.2.0)
Details
@github-actions
check-latest (ubuntu-latest, 10)
Details
@github-actions
check-latest (ubuntu-latest, 11)
Details
@github-actions
check-latest (ubuntu-latest, 12)
Details
@github-actions
check-latest (ubuntu-latest, 14)
Details
@github-actions
check-latest (windows-latest, 10)
Details
@github-actions
check-latest (windows-latest, 11)
Details
@github-actions
check-latest (windows-latest, 12)
Details
@github-actions
check-latest (windows-latest, 14)
Details
@github-actions
check-latest (macos-latest, 10)
Details
@github-actions
check-latest (macos-latest, 11)
Details
@github-actions
check-latest (macos-latest, 12)
Details
@github-actions
check-latest (macos-latest, 14)
Details
@github-actions
node-dist (ubuntu-latest, 11)
Details
@github-actions
node-dist (ubuntu-latest, 13)
Details
@github-actions
node-dist (windows-latest, 11)
Details
@github-actions
node-dist (windows-latest, 13)
Details
@github-actions
node-dist (macos-latest, 11)
Details
@github-actions
node-dist (macos-latest, 13)
Details
@github-actions
old-versions (ubuntu-latest)
Details
@github-actions
old-versions (windows-latest)
Details
@github-actions
old-versions (macos-latest)
Details
@github-actions
arch
Details
@gordey4doronin gordey4doronin deleted the gordey4doronin:gordey/support-lts-syntax branch Jun 30, 2021
@maxim-lobanov
Copy link
Collaborator

@maxim-lobanov maxim-lobanov commented Jun 30, 2021

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

@JimiC
Copy link

@JimiC 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

@skjnldsv skjnldsv commented Jun 30, 2021

Thank you everyone that worked on this! Great addition! 馃帀 馃 鉂わ笍

@JimiC
Copy link

@JimiC 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

@skjnldsv skjnldsv 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! 馃槙

Have a great day! 鈽锔

@JimiC
Copy link

@JimiC 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 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

@gordey4doronin gordey4doronin 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.

@mrlubos
Copy link

@mrlubos 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants