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.

Sign up for GitHub

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

Jump to bottom

Log node and npm versions #137

Merged
merged 8 commits into from
Apr 21, 2020
Merged

Log node and npm versions #137

merged 8 commits into from
Apr 21, 2020

Conversation

joshmgross
Copy link
Member

@joshmgross joshmgross commented Apr 16, 2020 •
edited
Loading

Resolves #90

This logs the node and npm versions that are installed by setup-node, following the pattern established by setup-go, see https://github.com/actions/setup-go/blob/v2-beta/src/main.ts#L54

Example

Run: https://github.com/actions/setup-node/pull/137/checks?check_run_id=606120085#step:4:5
Log output

C:\hostedtoolcache\windows\node\0.12.18\x64\node.exe --version
v0.12.18
C:\windows\system32\cmd.exe /D /S /C ""C:\Program Files\nodejs\npm.cmd" --version"
6.14.4

@joshmgross joshmgross added the enhancement New feature or request label Apr 16, 2020
@joshmgross joshmgross mentioned this pull request Apr 16, 2020
Closed
bryanmacfarlane
bryanmacfarlane reviewed Apr 16, 2020
View reviewed changes
src/setup-node.ts Outdated
@@ -17,6 +19,15 @@ async function run() {
await installer.getNode(version);
}

// Output version of node and npm that are being used
const nodePath = await io.which('node');
const nodeVersion = cp.execSync(`${nodePath} --version`);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't execSync return a buffer of bytes that you need toString() (safely)? See https://github.com/actions/setup-go/blob/master/src/main.ts#L56

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought template strings (ex ${nodePath} --version) already called .toString()

Copy link
Member

Choose a reason for hiding this comment

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

Ahhh - yeah. I think you're right. Just e2e test

bryanmacfarlane
bryanmacfarlane approved these changes Apr 16, 2020
View reviewed changes
src/setup-node.ts Outdated
@@ -17,6 +19,15 @@ async function run() {
await installer.getNode(version);
}

// Output version of node and npm that are being used
const nodePath = await io.which('node');
const nodeVersion = cp.execSync(`${nodePath} --version`);
Copy link
Member

Choose a reason for hiding this comment

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

Ahhh - yeah. I think you're right. Just e2e test

src/setup-node.ts Outdated

const npmPath = await io.which('npm');
const npmVersion = cp.execSync(`${npmPath} --version`);
console.log(`npm Version: ${npmVersion}`);
Copy link
Member

Choose a reason for hiding this comment

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

not sure why none of this would fail. On one hand I was thinking we should make all this best effort. But then I rethought that if this can't work, then the action didn't do it's job so it should fail. So I think it's fine how it is.

fishcharlie
fishcharlie approved these changes Apr 20, 2020
View reviewed changes
Copy link
Contributor

@fishcharlie fishcharlie left a comment

Choose a reason for hiding this comment

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

🎉 🎆

@fishcharlie
Copy link
Contributor

image

Looks like this new code might be failing on Windows??

@bryanmacfarlane
Copy link
Member

@joshmgross ☝️

@joshmgross
Copy link
Member Author

Thanks @fishcharlie, it was an issue with the spaces in the Windows Path, causing the path to not be interpreted correctly by cp.execSync

I've switched to @actions/exec which handles this edge case.

@fishcharlie
Copy link
Contributor

@joshmgross Perfect. Looks great! Thanks for this. Never seen "$ --version syntax before ("$ part). But I'm assuming it works fine. Would have suggested writing some tests just to verify, but not sure how you would really write tests cases around this.

Looking forward to seeing this be merged and released as part of this action. Thanks again!!

@joshmgross
Copy link
Member Author

Never seen "$ --version syntax before ("$ part).

Bug on my part, but should be fixed now.

Would have suggested writing some tests just to verify, but not sure how you would really write tests cases around this.

I think we would have to mock out exec to get any testing done, and at that point, I'm not sure what we would actually be testing. I'm letting the E2E test cover this case.

@joshmgross joshmgross merged commit e715d9a into master Apr 21, 2020
@joshmgross joshmgross deleted the joshmgross/log-node-version branch April 21, 2020 19:53
XhmikosR
XhmikosR reviewed Apr 22, 2020
View reviewed changes
src/setup-node.ts
@@ -1,4 +1,6 @@
import * as core from '@actions/core';
import * as exec from '@actions/exec';
import * as io from '@actions/io';
Copy link

Choose a reason for hiding this comment

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

Isn't this unused @joshmgross?

@XhmikosR XhmikosR mentioned this pull request Dec 2, 2020
Closed
@renovate renovate bot mentioned this pull request Jun 6, 2021
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XhmikosR XhmikosR XhmikosR left review comments

@bryanmacfarlane bryanmacfarlane bryanmacfarlane approved these changes

@fishcharlie fishcharlie fishcharlie approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Log Node.js version using
4 participants
@joshmgross @fishcharlie @bryanmacfarlane @XhmikosR