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

Print node, npm and yarn versions after installation #368

Merged
merged 10 commits into from Oct 3, 2022
Merged

Print node, npm and yarn versions after installation #368

merged 10 commits into from Oct 3, 2022

Conversation

havenchyk
Copy link
Contributor

Description:
Print node, npm and yarn version as it was before #147

Related issue:
#90

Check list:

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

src/main.ts Outdated Show resolved Hide resolved
@IvanZosimov
Copy link
Contributor

Hi, @havenchyk 👋 ! Thank you for your contribution! I'd suggest using getExecOutput() function instead of exec() to be able to format the information in the log and also to handle such situations when we want to notify user that there is no such util in the PATH (for instance the customer changed the PATH on the self-hosted runner and erased npm). Probably it'd be interesting to play with putting version information into the foldable group.

@IvanZosimov
Copy link
Contributor

📟 Just a gentle ping, @havenchyk

@dmitry-shibanov
Copy link
Contributor

Hello @havenchyk, just a gentle ping.

@havenchyk
Copy link
Contributor Author

hi guys, definitely I'll take a look: resolve conflicts today-tomorrow, thanks for reminders

@havenchyk havenchyk requested a review from a team July 31, 2022 12:32
@havenchyk
Copy link
Contributor Author

@dmitry-shibanov @IvanZosimov rebased, sorry for the delay

@havenchyk
Copy link
Contributor Author

oh, just noticed a comment about refactoring, let me address it

@havenchyk
Copy link
Contributor Author

@IvanZosimov can you please help me with the testing of changes? I see actions require some approval to run

@dmitry-shibanov
Copy link
Contributor

Hello @havenchyk. Could you please run npm run build and npm run format commands. Besides, I think we need to move yarn and npm to the try-cacth block too because it can cause issues like this for docker images when the tool does not exist or it is not supported.

@havenchyk
Copy link
Contributor Author

@dmitry-shibanov done. I think we need to run some action to verify what will the code do. Before I fix the tests, I know they will fail

@havenchyk
Copy link
Contributor Author

Guys, I need help with testing :) which job should I check to understand that the version is printed correctly?

src/main.ts Outdated Show resolved Hide resolved
__tests__/installer.test.ts Outdated Show resolved Hide resolved
__tests__/installer.test.ts Outdated Show resolved Hide resolved
@@ -249,6 +249,21 @@ describe('setup-node', () => {

let expPath = path.join(toolPath, 'bin');

expect(getExecOutputSpy).toHaveBeenCalledWith(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmitry-shibanov don't you think such check is better?

Choose a reason for hiding this comment

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

Thank you

core.setOutput(`${tool}-version`, output);
});

await Promise.all(promises);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't check it 😄

Choose a reason for hiding this comment

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

Thank yo

src/main.ts Outdated Show resolved Hide resolved
@havenchyk havenchyk requested review from nickmccurdy and removed request for nickmccurdy August 2, 2022 19:56
@havenchyk
Copy link
Contributor Author

@dmitry-shibanov please take a look once again

@dmitry-shibanov dmitry-shibanov merged commit c81d8ad into actions:main Oct 3, 2022
@dmitry-shibanov
Copy link
Contributor

Hello @havenchyk. Thank you for contribution. Sorry for the late response.

deining pushed a commit to deining/setup-node that referenced this pull request Nov 9, 2023
…ions#368)

Bumps [eslint-config-prettier](https://github.com/prettier/eslint-config-prettier) from 8.3.0 to 8.4.0.
- [Release notes](https://github.com/prettier/eslint-config-prettier/releases)
- [Changelog](https://github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md)
- [Commits](prettier/eslint-config-prettier@v8.3.0...v8.4.0)

---
updated-dependencies:
- dependency-name: eslint-config-prettier
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

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

7 participants