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

fix: add PR number for paged pull requests #18

Merged

Conversation

hicksjacobp
Copy link
Contributor

@hicksjacobp hicksjacobp commented May 4, 2022

We found that when a PR has a large number of reviews, that the processing of
the pull request will fail because it finds the PR number to be null (see line
994
). This is because the PR number is not included in the request for
subsequent pages.

Here's an example output run where we had an issue. You can see number is not requested.

21:26:59  Analyzing Pull Request Reviews for: mui PR: 7745
21:27:00  More pages of reviews. Gathering next batch.
21:27:00  Getting pull request reviews
21:27:00  curl -kw '%{http_code}' -s -X POST -H "authorization: Bearer ****" -H "content-type: application/json"   --data "    {
21:27:00    "query":"{ repository(owner:\"metasys\" name:\"mui\") { owner { login } name pullRequest(number:7745) { commits(first:1){  totalCount   }    timeline(first: 1) {  totalCount    }    comments(first: 1) {  totalCount    }    reviews(first: 100, after: \"Y3Vyc29yOnYyOpO5MjAxOS0wOC0wMlQwOTo1ODoyNC0wNTowMLkyMDE5LTA4LTAyVDA5OjU4OjIzLTA1OjAwzgACvo4=\") { totalCount pageInfo { hasNextPage endCursor } nodes { comments(first: 1) {    totalCount } } } } }}"

... and later

21:27:01  Analyzing Pull Request Reviews for: mui PR: null
21:27:03  More pages of reviews. Gathering next batch.
21:27:03  Getting pull request reviews
21:27:03  curl -kw '%{http_code}' -s -X POST -H "authorization: Bearer ****" -H "content-type: application/json"   --data "    {
21:27:03    "query":"{ repository(owner:\"metasys\" name:\"mui\") { owner { login } name pullRequest(number:null) { commits(first:1){  totalCount   }    timeline(first: 1) {  totalCount    }    comments(first: 1) {  totalCount    }    reviews(first: 100, after: \"Y3Vyc29yOnYyOpO5MjAxOS0wOC0wMlQxMDowMzozNy0wNTowMLkyMDE5LTA4LTAyVDEwOjAzOjM3LTA1OjAwzgACvvM=\") { totalCount pageInfo { hasNextPage endCursor } nodes { comments(first: 1) {    totalCount } } } } }}"
21:27:03      }"   "https://github.jci.com/api/graphql"
21:27:03  REVIEW DATA BLOCK:
21:27:03  {
21:27:03    "errors": [
21:27:03      {
21:27:03        "path": [
21:27:03          "query",
21:27:03          "repository",
21:27:03          "pullRequest",
21:27:03          "number"
21:27:03        ],
21:27:03        "extensions": {
21:27:03          "code": "argumentLiteralsIncompatible",
21:27:03          "typeName": "Field",
21:27:03          "argumentName": "number"
21:27:03        },
21:27:03        "locations": [
21:27:03          {
21:27:03            "line": 1,
21:27:03            "column": 65
21:27:03          }
21:27:03        ],
21:27:03        "message": "Argument 'number' on Field 'pullRequest' has an invalid value (null). Expected type 'Int!'."
21:27:03      }
21:27:03    ]
21:27:03  }

We found that when a PR has a large number of reviews, that the processing of
the pull request will fail because it finds the PR number to be null (see line
994). This is because the PR number is not included in the request for
subsequent pages.
@hicksjacobp hicksjacobp requested a review from a team as a code owner May 4, 2022 14:31
Copy link
Contributor

@admiralAwkbar admiralAwkbar left a comment

Choose a reason for hiding this comment

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

very nice catch!

@admiralAwkbar admiralAwkbar merged commit 13a2000 into mona-actions:main May 4, 2022
@hicksjacobp hicksjacobp deleted the hicksjacobp/fix-pr-paging branch May 4, 2022 16:41
@Djnjanetstiers Djnjanetstiers linked an issue May 2, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hello-world
2 participants