| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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 }}"
...
Sorry, something went wrong.
There was a problem hiding this comment.
@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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
All right, we'll just go with separate pull requests for now and see how it goes. Thanks!
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.)
Sorry, something went wrong.
There was a problem hiding this comment.
(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.)
Sorry, something went wrong.
There was a problem hiding this comment.
Still needs to be tested but this will hopefully now only open a pull request:
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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 }}
Sorry, something went wrong.
There was a problem hiding this comment.
I like the idea, but there a few small obstacles:
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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
- 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: updateAnd then use it as ${{ steps.update.outputs.version }}
Sorry, something went wrong.
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>
Update ESLint, Babel, remark, and so on. Run once a week. PR-URL: #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: #40644 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Update ESLint, Babel, remark, and so on. Run once a week. PR-URL: #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: #40644 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Update ESLint, Babel, remark, and so on. Run once a week. PR-URL: #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: #40644 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
| Back | FazBrowse Home | New Git URL |
Update ESLint, Babel, remark, and so on. Run once a week.