Skip to content

CI: PrAssignee.yml: .catch() and return the error object if 404 is encountered when checking collaborator status #58550

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 28, 2025
Merged

CI: PrAssignee.yml: .catch() and return the error object if 404 is encountered when checking collaborator status #58550

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions .github/workflows/PrAssignee.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ jobs:
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1
with:
retries: 5 # retry GitHub API requests up to 5 times, with exponential backoff
retry-exempt-status-codes: 403
retry-exempt-status-codes: "403,404"
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
retry-exempt-status-codes: "403,404"
retry-exempt-status-codes: "404"

# In the above list:
#
# - 404 is in the list because we will always hit a 404 when the PR author is not
# a committer. This 404 is normal and expected.
Comment on lines +41 to +45
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
retry-exempt-status-codes: "403,404"
# In the above list:
#
# - 404 is in the list because we will always hit a 404 when the PR author is not
# a committer. This 404 is normal and expected.

This isn't needed per the readme of plugin-retry.jl

Retries requests for server 4xx/5xx responses except 400, 401, 403, 404, 410, 422, and 451.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was actually wanting to retry on 400. In the past (although I don't have an example handy), I've seen GitHub give intermittent 400 in situations that were actually GitHub's fault server-side1.

So I'd want us to still retry on 400 - I'm fine with adding the other above-listed codes to our manual list. With that being said, the only one we actually know that we expect to see (in the normal/happy path) is 404, so really we could remove 403 (from our manual list) and just have 404 in our manual list.

Footnotes

  1. Which really means that GitHub should have been returning a 5xx, because it's a server-side problem on their end, but in the past I've sometimes gotten 400 from them in such situations. ↩

Copy link
Member

Choose a reason for hiding this comment

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

I made a PR to update the list and comment #58555

script: |
const oldPrAssignees = context.payload.pull_request.assignees
.map(obj => obj.login)
Expand All @@ -55,7 +59,26 @@ jobs:
headers: {
'X-GitHub-Api-Version': '2022-11-28'
}
})
}).catch((error) => {
// So, I'm not sure if the error is being thrown by `@octokit/request.js` or
// `@octokit/plugin-retry.js`. I suspect that the initial error is thrown by
// `request.js`, and is subsequently propagated by `plugin-retry.js`.
//
// Anyway, the docs for `request.js` say the following:
//
// > If an error occurs, the promise is rejected with an `error` object
//> containing 3 keys to help with debugging:
// >
// > - `error.status` The http response status code
// > - `error.request` The request options such as `method`, `url` and `data`
// > - `error.response` The http response object with `url`, `headers`, and `data`
//
// Source: https://github.com/octokit/request.js/blob/main/README.md#request
//
// So, inside this `.catch()`, we simply return that `error` object.

return error;
});
if (isCollaboratorResponseObj.status == 204) {
var isCollaborator = true;
} else if (isCollaboratorResponseObj.status == 404) {
Expand Down