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

tools: adjust script for use with 14.x #39362

Merged
merged 2 commits into from
Jul 14, 2021
Merged

tools: adjust script for use with 14.x #39362

merged 2 commits into from
Jul 14, 2021

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 12, 2021

find-inactive-collaborators.mjs works fine with Node.js 16.x, but GitHub
Actions currently use 14.x by default. This PR makes a small adjustment
so that it works with 14.x. (14.x does not accept URL objects as
paramemters for the cwd option in spawn().)

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Jul 12, 2021
@targos
Copy link
Member

targos commented Jul 12, 2021

Why not use 16.x in the action? This will never be run with node.js 14

@Trott
Copy link
Member Author

Trott commented Jul 12, 2021

Why not use 16.x in the action? This will never be run with node.js 14

I don't understand what you mean by "This will never be run with node.js 14". It runs with 14.x right now in the GitHub Action because that is the version that comes installed by default in GitHub Actions. https://github.com/nodejs/node/runs/3043788212?check_suite_focus=true I think I'm misunderstanding what you mean.

As for the question, though: If I add a step to the YAML workflow to install Node.js 16, then it will work, of course, and that was my first impulse. I went with this change instead for two reasons.

First, it's less overhead--just a small change to a script rather than downloading and installing Node.js every time (which is a slightly bigger but still small change to the YAML config but more "work" for the GitHub host).

Second, this doesn't require any further updates in the future, and if we do update it, it's really just a single straightforward update: remove the property added here. If we go with using setup-node to install Node.js 16, then we will need to change setup-node at some point or else eventually be running an EOL version of Node.js. I guess that's not exactly the end of the world, but on top of that, the possible changes are many. We could update the action to install a later version of Node.js, we could remove the installation of a newer version and go back to using the default version in the OS image used by GitHub Actions, we could switch to lts/*, etc.

So this seemed like a smaller temporary change with a more obvious path of what to do when it is no longer needed, and smaller downside if we never undo it.

@targos
Copy link
Member

targos commented Jul 12, 2021

I don't understand what you mean by "This will never be run with node.js 14". It runs with 14.x right now in the GitHub Action because that is the version that comes installed by default in GitHub Actions.

I should have added "if we do it (use 16.x in the action)".

more "work" for the GitHub host

A very smaller amount of work. The host usually doesn't need to download anything because GitHub runners have common Node.js versions in cache.

If we go with using setup-node to install Node.js 16, then we will need to change setup-node at some point or else eventually be running an EOL version of Node.js

It's true, but we will already need to do it for other existing workflows

In general, I prefer to have control on the versions of software that are used in CI, instead of relying on defaults.
Those are just my opinions, I'm fine with whatever lands.

@Trott
Copy link
Member Author

Trott commented Jul 12, 2021

In general, I prefer to have control on the versions of software that are used in CI, instead of relying on defaults.

I don't feel strongly about it, so I'll go ahead and change this to a workflow adjustment instead.

@Trott Trott force-pushed the for-14 branch 2 times, most recently from 2a98bed to 2c86a57 Compare July 12, 2021 15:44
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

LGTM with or without my suggestion.

.github/workflows/find-inactive-collaborators.yml Outdated Show resolved Hide resolved
find-inactive-collaborators.mjs works fine with Node.js 16.x, but GitHub
Actions currently use 14.x by default.

PR-URL: nodejs#39362
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
GitHub Action workflows can be told to clone a certain number of commits
or else everything. Change find-inactive-collaborators to take a number
of commits to examine rather than a date range so that the parameter can
be used in GitHub Actions.

PR-URL: nodejs#39362
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jul 14, 2021

Landed in efd40ea...1bb660e

@Trott Trott merged commit 1bb660e into nodejs:master Jul 14, 2021
@Trott Trott deleted the for-14 branch July 14, 2021 11:36
targos pushed a commit that referenced this pull request Jul 17, 2021
find-inactive-collaborators.mjs works fine with Node.js 16.x, but GitHub
Actions currently use 14.x by default.

PR-URL: #39362
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 17, 2021
GitHub Action workflows can be told to clone a certain number of commits
or else everything. Change find-inactive-collaborators to take a number
of commits to examine rather than a date range so that the parameter can
be used in GitHub Actions.

PR-URL: #39362
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Jul 26, 2021
BethGriggs pushed a commit that referenced this pull request Jul 29, 2021
find-inactive-collaborators.mjs works fine with Node.js 16.x, but GitHub
Actions currently use 14.x by default.

PR-URL: #39362
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
BethGriggs pushed a commit that referenced this pull request Jul 29, 2021
GitHub Action workflows can be told to clone a certain number of commits
or else everything. Change find-inactive-collaborators to take a number
of commits to examine rather than a date range so that the parameter can
be used in GitHub Actions.

PR-URL: #39362
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Sep 4, 2021
find-inactive-collaborators.mjs works fine with Node.js 16.x, but GitHub
Actions currently use 14.x by default.

PR-URL: #39362
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Sep 4, 2021
GitHub Action workflows can be told to clone a certain number of commits
or else everything. Change find-inactive-collaborators to take a number
of commits to examine rather than a date range so that the parameter can
be used in GitHub Actions.

PR-URL: #39362
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants