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

Read version from .nvmrc or .tool-versions config #222

Closed

Conversation

matoakley
Copy link

@matoakley matoakley commented Jan 1, 2021

NVM and asdf are popular tools for managing node (and other dependency) versions in projects. These tools provide the ability to specify the version number in either a .nvmrc or .tool-versions file and have the management tool automatically install and configure that version.

This patch (shamelessly plagiarised and adapted from setup-ruby) will attempt to set the version from the .nvmrc or .tool-versions file if not specified explicitly. If it is unable to find and parse a version then it will return a blank string and fallback to the original logic to use the architecture's pre-installed node.

To be clear, the order of precedence is:

  1. Use the version explicitly set in the version or node-version option,
  2. Attempt to read the version from .nvmrc if the file is present,
  3. Attempt to read the version from .tool-versions if the file is present,
  4. Fallback to the system node.

By enabling the action to read from these common configuration files we reduce coupling and allow developers to manage their node version from a single location within their codebase.

This patch provides the enhancement discussed in #32 .

Finally, this is the first time that I've worked with Typescript (I'm a rubyist at heart) so I'm open to code review and suggestions to improve the code itself.

Happy New Year! 🎉

@matoakley matoakley marked this pull request as ready for review January 1, 2021 01:03
@matoakley matoakley changed the title Read version from .nvmrc or .tool-versions Read version from .nvmrc or .tool-versions config Jan 1, 2021
@bryanmacfarlane bryanmacfarlane added the enhancement New feature or request label Jan 20, 2021
@@ -4742,6 +4740,29 @@ function isGhes() {
const ghUrl = new url_1.URL(process.env['GITHUB_SERVER_URL'] || 'https://github.com');
return ghUrl.hostname.toUpperCase() !== 'GITHUB.COM';
}
function parseNodeVersion() {
let nodeVersion = core.getInput('node-version') || core.getInput('version');

Choose a reason for hiding this comment

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

just wanted to ask here whether you want to check for a falsy value or just null or undefined? If the latter:

Suggested change
let nodeVersion = core.getInput('node-version') || core.getInput('version');
let nodeVersion = core.getInput('node-version') ?? core.getInput('version');

const nodeLine = toolVersions
.split(/\r?\n/)
.filter(e => e.match(/^nodejs\s/))[0];
nodeVersion = nodeLine.match(/^nodejs\s+(.+)$/)![1];

Choose a reason for hiding this comment

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

.match() returns RegExpMatchArray | null, so rather than using definite assignment assertion (!), perhaps use the optional chain (?) to query the RegExpMatchArray. If nothing is returned here, it will assign undefined to nodeVersion.

This does then throw into question the flow of these statements, because if I have a .tool-versions with the nodejs key in (I think that's what you're testing?) but no value, then it will return undefined.

I would revisit this by allowing TS to guide you with the types & then making sure that a string is definitely being returned.

Suggested change
nodeVersion = nodeLine.match(/^nodejs\s+(.+)$/)![1];
nodeVersion = nodeLine.match(/^nodejs\s+(.+)$/)?.[1];

@jasonkarns
Copy link

Looks like a dupe of #209 ?

Just commenting since it's highly related: https://github.com/nodenv/actions-node-version is an action that reads .node-version file (leveraging nodenv) and exposes it as an output that can be used as an input to the setup-node action. This would be a workaround until/unless setup-node gains the ability to do it automatically.

@ruscon
Copy link

ruscon commented Mar 13, 2021

@matoakley @jamiehaywood Is there anything I can do to complete this feature?

@jamiehaywood
Copy link

@ruscon unfortunately I'm not a maintainer, I just added a review because I'm keen to get this feature into setup-node. I think there are 2 or 3 PRs covering the same feature floating around, so it might be worth having a look on them as well, and seeing the status of that.

@ruscon
Copy link

ruscon commented Mar 13, 2021

this solution is better
#239

@matoakley
Copy link
Author

@jamiehaywood Thanks for the review it's much appreciated 👍

Sadly I've lost hope on this PR given that the project appears to be all but abandoned by its maintainers. As has been pointed out, there are now at least 3 open PRs offering a solution to this problem and there seems to be high demand for the feature.

@ruscon I agree that #239 would probably be a better all-round solution but whether it is "better" is subjective. For example, it wouldn't have solved my problem as the patch does not lookup the .tool-versions file which https://github.com/asdf-vm/asdf uses.

In the 3 months since I submitted this PR I have migrated my development environment to Docker and have resigned myself to managing the Node version manually for Github Actions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants