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

Don't look at check run results in other repositories #807

Merged

Conversation

iainlane
Copy link
Contributor

@iainlane iainlane commented Aug 1, 2024

We are notified about check run completions via the check_run webhook event. This event contains a pull_requests field, which is a list of PRs that contain the SHA which just got checked:

      "pull_requests": [
        {
          "url": "https://api.github.com/repos/myorg/myrepo/pulls/1337",
          "id": 333333333,
          "number": 1337,
          "head": {
            "ref": "some-branch",
            "sha": "abcdef0123456789abcdef0123456789abcdef01",
            "repo": {
              "id": 11111111,
              "url": "https://api.github.com/repos/myorg/myrepo",
              "name": "myrepo"
            }
          },
          "base": {
            "ref": "main",
            "sha": "0101010101010101010101010101010101010101",
            "repo": {
              "id": 111111111,
              "url": "https://api.github.com/repos/myorg/myrepo",
              "name": "myrepo"
            }
          }
        }
      ],

However (mainly but not exclusively), in the case of public repositories, this list can contain PRs in other repositories:

    "pull_requests": [
      {
        "url": "https://api.github.com/repos/forkowner/myrepo/pulls/3",
        "id": 1234567890,
        "number": 3,
        "head": {
          "ref": "main",
          "sha": "6666666666666666666666666666666666666666",
          "repo": {
            "id": 111111111,
            "url": "https://api.github.com/repos/myorg/myrepo",
            "name": "myrepo"
          }
        },
        "base": {
          "ref": "main",
          "sha": "8888888888888888888888888888888888888888",
          "repo": {
            "id": 555555555,
            "url": "https://api.github.com/repos/forkowner/myrepo",
            "name": "myrepo"
          }
        }
      }
    ]

...this is because people can fork the repository and then create PRs from our repo into theirs. These PRs will contain SHAs from our repository and this causes them to appear in the pull_requests list of the check run, since they will share the same check run status in any repo: the check run is attached to the commit itself.

These PRs should be ignored for our purposes. We aren't evaluating a policy on the remote repo. When a check_run completes, we are only interested in PRs for our own repo.

@iainlane
Copy link
Contributor Author

iainlane commented Aug 1, 2024

I was reviewing our logs when working on #794 earlier and noticed a lot of instances of this message

failed to create evaluation context: failed to load pull request details: Could not resolve to a PullRequest with the number of <implausibly low number>

this turned out to be the reason!

We are notified about check run completions via the `check_run` webhook
event. This event contains a `pull_requests` field, which is a list of
PRs that contain the SHA which just got checked:

```json
      "pull_requests": [
        {
          "url": "https://api.github.com/repos/myorg/myrepo/pulls/1337",
          "id": 0000000000,
          "number": 1337,
          "head": {
            "ref": "some-branch",
            "sha": "abcdef0123456789abcdef0123456789abcdef01",
            "repo": {
              "id": 11111111,
              "url": "https://api.github.com/repos/myorg/myrepo",
              "name": "myrepo"
            }
          },
          "base": {
            "ref": "main",
            "sha": "0101010101010101010101010101010101010101",
            "repo": {
              "id": 111111111,
              "url": "https://api.github.com/repos/myorg/myrepo",
              "name": "myrepo"
            }
          }
        }
      ],
```

However (mainly but not exclusively), in the case of public
repositories, this list can contain PRs in other repositories:

```json
    "pull_requests": [
      {
        "url": "https://api.github.com/repos/forkowner/myrepo/pulls/3",
        "id": 1234567890,
        "number": 3,
        "head": {
          "ref": "main",
          "sha": "6666666666666666666666666666666666666666",
          "repo": {
            "id": 111111111,
            "url": "https://api.github.com/repos/myorg/myrepo",
            "name": "myrepo"
          }
        },
        "base": {
          "ref": "main",
          "sha": "8888888888888888888888888888888888888888",
          "repo": {
            "id": 555555555,
            "url": "https://api.github.com/repos/forkowner/myrepo",
            "name": "myrepo"
          }
        }
      }
    ]
```

...this is because people can fork the repository and then create PRs
_from_ our repo _into_ theirs. These PRs will contain SHAs from our
repository and this causes them to appear in the `pull_requests` list of
the check run, since they will share the same check run status in any
repo: the check run is attached to the commit itself.

These PRs should be ignored for our purposes. We aren't evaluating a
policy on the remote repo. When a `check_run` completes, we are only
interested in PRs for our own repo.
@iainlane iainlane force-pushed the iainlane/check-run-ignore-other-prs branch from eb9f859 to ff55775 Compare August 1, 2024 10:33
@iainlane iainlane marked this pull request as ready for review August 1, 2024 10:33
Copy link
Member

@bluekeyes bluekeyes left a comment

Choose a reason for hiding this comment

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

Good catch, I don't think we've seen this before

@bluekeyes bluekeyes merged commit 536d202 into palantir:develop Aug 2, 2024
8 checks passed
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.

2 participants