FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build: add GitHub Action to update tools modules by Trott · Pull Request #40644 · nodejs/node · GitHub

/ node Public

build: add GitHub Action to update tools modules - #40644

Closed
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:tools-update
Closed

build: add GitHub Action to update tools modules#40644
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:tools-update

Conversation

Trott commented Oct 29, 2021

Copy link
Copy Markdown
Member

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

nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Oct 29, 2021
Comment thread .github/workflows/tools.yml Outdated
Comment thread .github/workflows/tools.yml Outdated
Trott force-pushed the tools-update branch 2 times, most recently from de35b7a to 9bbc793 Compare October 30, 2021 05:25
Comment thread .github/workflows/tools.yml Outdated
Comment on lines 15 to 19

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Mesteery Oct 30, 2021
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

Mesteery Oct 30, 2021
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Trott Oct 31, 2021
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.)

Trott Oct 31, 2021
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

(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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 force-pushed the tools-update branch 2 times, most recently from bc94622 to 82c2751 Compare November 3, 2021 04:44
Comment thread .github/workflows/tools.yml Outdated

Mesteery Nov 3, 2021
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 }}

Trott Nov 4, 2021
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Mesteery Nov 4, 2021
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 force-pushed the tools-update branch 2 times, most recently from 5792c98 to 2924c48 Compare November 5, 2021 04:32
Trott added 2 commits November 8, 2021 11:47
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 commented Nov 8, 2021

Copy link
Copy Markdown
Member Author

Landed in df2fe87...04451d8

Trott closed this Nov 8, 2021
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 mentioned this pull request Feb 1, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL