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: update ESLint to 8.2.0 #40734

Closed
wants to merge 1 commit into from
Closed

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Nov 6, 2021

Update ESLint to 8.2.0

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Nov 6, 2021
@targos targos added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 6, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2021

Fast-track has been requested by @targos. Please 👍 to approve.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Rubber-stamp LGTM

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 6, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 6, 2021
@targos
Copy link
Member

targos commented Nov 6, 2021

@targos
Copy link
Member

targos commented Nov 6, 2021

Last lines of the logs:

To finish landing:
1. Run `git push origin master`
2. Post "Landed in 4d01716bcefe" in https://github.com/nodejs/node/pull/40734
+ [ -z --oneCommitMax ]
+ git log -1 --pretty=format:%s
+ git log -1 --pretty=format:%b
+ jq -n --arg title tools: update ESLint to 8.2.0 --arg body PR-URL: https://github.com/nodejs/node/pull/40734
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Voltrex <[email protected]>
Reviewed-By: Rich Trott <[email protected]> {merge_method:"squash",commit_title:$title,commit_message:$body}
+ cat output.json
{
  "merge_method": "squash",
  "commit_title": "tools: update ESLint to 8.2.0",
  "commit_message": "PR-URL: https://github.com/nodejs/node/pull/40734\nReviewed-By: Michaël Zasso <[email protected]>\nReviewed-By: Voltrex <[email protected]>\nReviewed-By: Rich Trott <[email protected]>"
}
+ mergeUrl 40734
+ echo https://api.github.com/repos/nodejs/node/pulls/40734/merge
+ gitHubCurl https://api.github.com/repos/nodejs/node/pulls/40734/merge PUT --data @output.json
+ url=https://api.github.com/repos/nodejs/node/pulls/40734/merge
+ method=PUT
+ shift 2
+ curl -fsL --request PUT --url https://api.github.com/repos/nodejs/node/pulls/40734/merge --header authorization: *** --header content-type: application/json --data @output.json
Error: Process completed with exit code 22.

@targos
Copy link
Member

targos commented Nov 6, 2021

Exit code 22 means that the HTTP response's code was >=400

@targos
Copy link
Member

targos commented Nov 6, 2021

I tried locally, the response code is 404

@Trott
Copy link
Member

Trott commented Nov 6, 2021

Commit queue failed (https://github.com/nodejs/node/runs/4125859812?check_suite_focus=true) but it only commented on #40720

@aduh95 Probably related to 80b8440?

Trott pushed a commit that referenced this pull request Nov 6, 2021
PR-URL: #40734
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Voltrex <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 6, 2021

Landed in 1977afd

@Trott Trott closed this Nov 6, 2021
@targos
Copy link
Member

targos commented Nov 6, 2021

@aduh95 ^

@aduh95
Copy link
Contributor

aduh95 commented Nov 6, 2021

I tried landing #40664 (comment) using the CQ, and it crashed. However, trying the command manually succeeded for me, I wasn't able to reproduce the 404. Maybe it's token that we use is not allowed to merge PRs?

@targos
Copy link
Member

targos commented Nov 6, 2021

I think we use the default token provided by GitHub. It's supposed to have read/write access to pull requests: https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

@aduh95
Copy link
Contributor

aduh95 commented Nov 6, 2021

We don't, we set a special one from the repo secrets:

# A personal token is required because pushing with GITHUB_TOKEN will
# prevent commits from running CI after they land. It needs
# to be set here because `checkout` configures GitHub authentication
# for push as well.
token: ${{ secrets.GH_USER_TOKEN }}

@targos
Copy link
Member

targos commented Nov 6, 2021

That's not the one we pass to commit-queue.sh:

run: ./tools/actions/commit-queue.sh ${OWNER} ${REPOSITORY} ${{ secrets.GITHUB_TOKEN }} $(echo '${{ steps.get_mergable_pull_requests.outputs.data }}' | jq '.repository.pullRequests.nodes | map(.number) | .[]')

@aduh95
Copy link
Contributor

aduh95 commented Nov 6, 2021

According to the Set up job section, it does have write permissions:

GITHUB_TOKEN Permissions
  Actions: write
  Checks: write
  Contents: write
  Deployments: write
  Discussions: write
  Issues: write
  Metadata: read
  Packages: write
  Pages: write
  PullRequests: write
  RepositoryProjects: write
  SecurityEvents: write
  Statuses: write

Anyway it's very surprising the CQ was working on node-auto-test but doesn't on nodejs/node. Maybe it's related to one of the branch protection rules? None are setup on node-auto-test.

@targos
Copy link
Member

targos commented Nov 6, 2021

Maybe it's related to one of the branch protection rules?

Mmmh, that's possible. We do have a rule that only allows members of @nodejs/collaborators to push to the master branch. That makes me wonder how the commit queue ever worked, because obviously the github-actions bot is not a member of that team.

@aduh95
Copy link
Contributor

aduh95 commented Nov 6, 2021

That makes me wonder how the commit queue ever worked, because obviously the github-actions bot is not a member of that team.

That's probably when the GH_USER_TOKEN is used, according the comment above.

@targos
Copy link
Member

targos commented Nov 6, 2021

Oh I see, the nodejs-github-bot account is in the collaborators team... Then the solution is probably to pass its token to the script instead of the default one.

@targos
Copy link
Member

targos commented Nov 6, 2021

I explicitly added the bot to the list of who can push. I'd remove it from collaborators but I don't know if it would break something else.
image

@lpinca lpinca deleted the update/eslint branch November 6, 2021 18:53
targos pushed a commit that referenced this pull request Nov 8, 2021
PR-URL: #40734
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Voltrex <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request Nov 8, 2021
BethGriggs pushed a commit that referenced this pull request Nov 25, 2021
PR-URL: #40734
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Voltrex <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Nov 26, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants