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

build: add GitHub Action to update tools modules #40644

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 29, 2021

Update ESLint, Babel, remark, and so on. Run once a week.

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Oct 29, 2021
.github/workflows/tools.yml Outdated Show resolved Hide resolved
@Trott Trott force-pushed the tools-update branch 2 times, most recently from de35b7a to 9bbc793 Compare October 30, 2021 05:25
Comment on lines 15 to 19
- run: tools/update-eslint.sh
- run: tools/update-babel-eslint.sh
- run: |
(cd tools/lint-md && rm -rf package-lock.json node_modules && npm install --ignore-scripts)
make lint-md-rollup
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I like this. I would prefer to have separate commits, one for each tool, with descriptive messages like "tools: update ESLint to x.x.x" as we have now.

Copy link
Contributor

@Mesteery Mesteery Oct 30, 2021

Choose a reason for hiding this comment

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

I agree with lpinca, and by the way, with a matrix it is done quite cleanly.

   strategy:
     fail-fast: false # Prevent other jobs from aborting if one fails
     matrix:
       include:
         - id: eslint
           run: tools/update-eslint.sh
         ...
   steps:
     ...
     - run: |
         ${{ matrix.run }}
         echo "NEW_VERSION=..." >> $GITHUB_ENV
     - uses: gr2m/create-or-update-pull-request-action@v1  # Create a PR or update the Action's existing PR
       ...
       with:
         body: "This is an automated update of ${{ matrix.id }} to ${{ env.NEW_VERSION }}."
         branch: "actions/tools-update-${{ matrix.id }}"  # Custom branch *just* for this Action.
         commit-message: "tools: update ${{ matrix.id }} to ${{ env.NEW_VERSION }}"
         title: "tools: update ${{ matrix.id }} to ${{ env.NEW_VERSION }}"
         ...

Copy link
Member Author

Choose a reason for hiding this comment

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

@Mesteery I think that will do it as separate commits, but also separate pull requests. That will work, but I wonder if there's a straightforward way to have them be separate commits but a single pull request. Otherwise, especially as we add tools, the number of pull requests could get annoying. One pull request a week covering the tools directory but splitting each tool into its own commit (so individual things can be removed or reverted if they are problematic) would be my preference.

Copy link
Contributor

@Mesteery Mesteery Oct 30, 2021

Choose a reason for hiding this comment

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

Oh yes, indeed, I misread it, I thought he was suggesting to do it as separate pull requests.

I wonder if there's a straightforward way to have them be separate commits but a single pull request.

It's possible with peter-evans/create-pull-request if I understand correctly, but IMO it is not very pratical.

Copy link
Member Author

Choose a reason for hiding this comment

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

All right, we'll just go with separate pull requests for now and see how it goes. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, all set up (but not yet tested) with a matrix. (Thanks, @Mesteery!) I'm still pondering whether or how to get versions into the commit log. (This doesn't update only when the tool itself has a version bump. It updates dependencies even if the tool itself is the same version. So it's not as simple as, for example, checking to see what version of ESLint is installed.) Hopefully that's not a dealbreaker.

Copy link
Member

Choose a reason for hiding this comment

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

It's good to keep our tools up to date, but isn's once a week a bit too much? I'm pretty sure we will get at least one pull request every time this is run, because the ESLint and Babel dependency trees often have updates.

Copy link
Member Author

@Trott Trott Oct 31, 2021

Choose a reason for hiding this comment

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

It's good to keep our tools up to date, but isn's once a week a bit too much? I'm pretty sure we will get at least one pull request every time this is run, because the ESLint and Babel dependency trees often have updates.

Yes, it will almost certainly update once a week. (The remark dependencies are active too in lint-md too.) Would you be more comfortable with once every two weeks instead or something like that?

Maybe we can run npm outdated (or some equivalent check if npm outdated won't work in all of these situations) and only update if a top-level dependency is out of date. My hope was to keep all the transitive dependencies up to date, as that has mattered for remark linting. But we can not worry about that for now. (Maybe we can talk about auto-landing these kinds of PRs if all tests pass to minimize the maintenance burden and then running updates frequently for transitive dependencies is not a big deal.)

Copy link
Member Author

@Trott Trott Oct 31, 2021

Choose a reason for hiding this comment

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

(Also, running npm outdated or something similar is it allows us to get the version number to include in the commit messages and pR text.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Still needs to be tested but this will hopefully now only open a pull request:

  • if eslint has been updated
  • if @babel/eslint-parser has been updated
  • if a direct dependency of lint-md has been updated

It will open one pull request for each bullet point above, so up to 3 are possible, but will probably usually be zero or one.

@Trott Trott force-pushed the tools-update branch 2 times, most recently from bc94622 to 82c2751 Compare November 3, 2021 04:44
GITHUB_TOKEN: ${{ secrets.GH_USER_TOKEN }}
with:
author: Node.js GitHub Bot <github-bot@iojs.org>
body: "This is an automated update of ${{ matrix.id }} to ${{ env.NEW_VERSION }}."
Copy link
Contributor

@Mesteery Mesteery Nov 3, 2021

Choose a reason for hiding this comment

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

Personally I was thinking of something like

This is an automated update of eslint and its dependencies. Updated:
`eslint@x.y.z` to `x.y.z1`
`eslint-plugin-markdown@x.y.z` to `x.y.z1`

or

This is an automated update of eslint and its dependencies. Updated:
`eslint-plugin-markdown@x.y.z` to `x.y.z1`
matrix:
  include:
    - id: eslint
      packages: [eslint, eslint-plugin-markdown] # or '["eslint",...]', I don't know if Actions stringifies this
      before: cd tools
      update: tools/update-eslint.sh
  - run: |
      ${{ matrix.before }}
      NEW_VERSIONS=$(npm outdated --json | jq -r '.[] | select(.current != .wanted and .package as $p | ${{ matrix.packages }} | index($p)) | "`" + .package + "@" + .current + "` to `" + .wanted + "`"')
      if [ "$NEW_VERSIONS" != "" ]; then
        echo "NEW_VERSIONS=$NEW_VERSIONS" >> $GITHUB_ENV
        ${{ matrix.update }}
      fi

  - if: env.NEW_VERSIONS
    ...
    body: |
      This is an automated update of ${{ matrix.id }} and its dependencies. Updated:
      ${{ env.NEW_VERSIONS }}

Copy link
Member Author

@Trott Trott Nov 4, 2021

Choose a reason for hiding this comment

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

I like the idea, but there a few small obstacles:

  1. npm outdated will not work for eslint or @babel/eslint-parser the way we have them installed. That's why I'm doing things the way I am in those instances.
  2. I don't think the passing of a multiline value for the environment variable works in GitHub Actions. (I could be wrong about that, but that's what I seem to recall. If $NEW_VERSIONS contains newline characters, then "NEW_VERSIONS=$NEW_VERSIONS" won't work.)

For lint-md, the format you propose would be better than what I'm doing, as the way I'm doing it will result in lines longer than 72 characters frequently.

Copy link
Member Author

Choose a reason for hiding this comment

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

2. I don't think the passing of a multiline value for the environment variable works in GitHub Actions. (I could be wrong about that, but that's what I seem to recall. If $NEW_VERSIONS contains newline characters, then "NEW_VERSIONS=$NEW_VERSIONS" won't work.)

I suppose I can use some other character as a placeholder for the newline and then convert it in the relevant step.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the new versions are in a string with a format like "abc@1.2.3 xyz@4.5.6 foobar@42.0.0" then I can convert the spaces to newlines and the problem is solved.

Copy link
Contributor

@Mesteery Mesteery Nov 4, 2021

Choose a reason for hiding this comment

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

npm outdated will not work for eslint or @babel/eslint-parser the way we have them installed. That's why I'm doing things the way I am in those instances.

Ah

  1. I don't think the passing of a multiline value for the environment variable works in GitHub Actions. (I could be wrong about that, but that's what I seem to recall. If $NEW_VERSIONS contains newline characters, then "NEW_VERSIONS=$NEW_VERSIONS" won't work.)

Indeed, but it is possible by using outputs and escaping new lines. The runner will automatically take care of unescaping it:

  - run: |
      ...
        echo "::set-output name=version::${NEW_VERSIONS//$'\n'/'%0A'}"
      ...
    id: update

And then use it as ${{ steps.update.outputs.version }}

@Trott Trott force-pushed the tools-update branch 2 times, most recently from 5792c98 to 2924c48 Compare November 5, 2021 04:32
Update ESLint, Babel, remark, and so on. Run once a week.

PR-URL: nodejs#40644
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
The --production flag has no effect in this situation. Remove it.

Add --ignore-scripts as a precaution.

PR-URL: nodejs#40644
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@Trott
Copy link
Member Author

Trott commented Nov 8, 2021

Landed in df2fe87...04451d8

@Trott Trott closed this Nov 8, 2021
@Trott Trott deleted the tools-update branch November 8, 2021 19:49
targos pushed a commit that referenced this pull request Nov 21, 2021
Update ESLint, Babel, remark, and so on. Run once a week.

PR-URL: #40644
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Nov 21, 2021
The --production flag has no effect in this situation. Remove it.

Add --ignore-scripts as a precaution.

PR-URL: #40644
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
Update ESLint, Babel, remark, and so on. Run once a week.

PR-URL: #40644
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
The --production flag has no effect in this situation. Remove it.

Add --ignore-scripts as a precaution.

PR-URL: #40644
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
Update ESLint, Babel, remark, and so on. Run once a week.

PR-URL: #40644
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
The --production flag has no effect in this situation. Remove it.

Add --ignore-scripts as a precaution.

PR-URL: #40644
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants