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

PR Triage workflow fails on PRs from forks after the first approval #570

Assignees
Labels
bug Something isn't working

Comments

@laurelmay
Copy link
Contributor

laurelmay commented Aug 16, 2022

Description

The PR triage workflow is meant to facilitate and automate the review request process. When a contributor opens a PR, it requests review from an internal team and then continues to do that until the review count threshold has been met for that repository. To do this, it uses two different tokens:

  1. ${{ secrets.GITHUB_TOKEN }} - This is used for the GraphQL query and only needs pull-requests: read permissions
  2. A temporary token generated using ${{ secrets.APP_ID }} and ${{ secrets.APP_PRIVATE_KEY }}

Because (1) only uses read permissions and is the default token in a GitHub Actions workflow, it is available in all workflow executions; however, because (2) is derived from repository secrets which are only available in certain contexts, the secrets may not always be available.

So the situation in which the repository secrets aren't available can be loosely summarized as "when the secret could be exposed in an untrusted context". And primarily, this means that the secret never gets passed to forks but that it will get passed to workflows that execute against the base repo (this one).

The PR triage workflow starts out with a pull_request_target-triggered event: the PR being opened. pull_request_target events run against the base repository and specifically (if I recall correctly) the default branch of the repository (it might be the base branch of the PR but that's not a critical difference here). This means that we always run a trusted version of the workflow. Because that is on the base repository on the default branch and can't be modified within a PR, the repository secrets will be passed to that workflow (there is also slightly different behavior for ${{ secrets.GITHUB_TOKEN }} but that doesn't really matter here). This means that regardless of who opens a PR, what repo the head branch is on, or whether or not the user has modified the workflow file, the trusted version of the workflow file is executed with access to all the required secrets. The workflow will initially request a review from our internal team.

The invocation of the PR triage workflow is a pull_request_review event. This occurs when a user has left an approve/reject/comment review on a pull request, which is very useful for our workflow. We want to make sure that as long as people are approving and commenting that we keep asking our team for reviews (because we require more than one). We skip execution on a reject, but that's not particularly relevant here. The unfortunate thing about pull_request_review is that it is run in the context of the head branch/repository (the fork) and not the base branch/repository. This means that it gets a read-only version of ${{ secrets.GITHUB_TOKEN }} (which is okay) but it does not get any of the other repository secrets (APP_ID and APP_PRIVATE_KEY) when the PR comes from a fork. The app token is required because the specific permissions required to request review from a team are never available on the GITHUB_TOKEN.

The vast majority of our PRs at the moment come from within our team; users who have write access to the base repository and whose PRs will therefore have access to the repository secrets. So for internal team members, the bot will happily keep requesting reviews as long as necessary. For any PR that originates from a fork, the bot unfortunately never gets triggered (you can see in the workflow execution logs that the GraphQL step works but the app login step is where the failure occurs).

This makes the bot less-than-very-useful. Internal contributors know how to keep asking the team for reviews but an external contributor may not see a path forward to ask for a second review. It also results in the PR appearing to have a failing status check (technically, something is failing but it's not the contributor's fault).

Note: This issue impacts all our repositories that have this workflow; however, I am opening the issue here because it is our most active at the moment and is where we first noticed the issue.

For PRs where this occurred, see #564 and #568.

Environment

No response

Steps to Reproduce

  1. Fork EasyDynamics/oscal-react-library
  2. Create a branch and make modifications
  3. Open a PR against EasyDynamics/oscal-react-library
  4. Receive 1 approval or comment on the PR

Expected behavior

The PR Triage workflow is initiated and requests review from the team again

Actual Behavior

The PR Triage workflow fails, resulting in a ❌ status check on the PR and failing to request a review from the internal team.

Additional Notes

Resolving this will require us to either make changes to the PR Triage workflow, our branch protection rules (to decrease the number of required reviewers), find another tool to do this action, or stop using the PR Triage workflow.

Changes to the PR Triage workflow

If we changed the workflow to use some event that runs in the context of the base repository, then the workflow should resume working for external contributors. What we must not do is pass secrets to Actions workflows on forks. Any change here would need to be something to the workflow itself.

We could also skip execution of the workflow if it is running in the context of a fork to at least prevent failures; however, that would not result in a review request.

Alternative tools

I don't know if "mergify" can handle this type of workflow but it seems to be a popular tool. Additionally, we may be able to accomplish something similar using repository webhooks and a service that handled them and reused our same app but that feels like an overengineered solution.

Removing the workflow

We could just stop using the PR triage workflow; however, then we would potentially go back to "missing" PRs that still require a second review. We could, along with this, change the branch protection rules to only require a single review. Then paired with a CODEOWNERS file, we could accomplish requesting all required reviews without needing to use a GitHub Actions workflow at all.

@laurelmay laurelmay added the bug Something isn't working label Aug 16, 2022
@tuckerzp
Copy link
Contributor

tuckerzp commented Aug 16, 2022

Removing the workflow

While needing 2 reviews at times when we have had only 3 reviewers was a bit annoying, I do like needing more than 1 person to approve. I think in general it leads to better code.

With that said, I do think that outside contributor's prs not getting reviewed and even worse the workflow telling them their pr failed is not good. If that means we only require 1 review, I think it may be worth it.

@tuckerzp tuckerzp self-assigned this Apr 25, 2023
@tuckerzp
Copy link
Contributor

Now that I have a chance to implement this, I still think deleting the workflow is the best past forward. For now, I will keep the required 2 reviewers. I think that the team is generally on top of pr's so we should not miss much.

Bronstrom pushed a commit that referenced this issue Apr 26, 2023
To stop unnecessary failures on prs from forks, this removes the triage
workflow

closes #570
@tuckerzp tuckerzp reopened this Apr 27, 2023
laurelmay pushed a commit to EasyDynamics/oscal-rest-service that referenced this issue Apr 27, 2023
To stop unnecessary failures on prs from forks, this removes the triage
workflow

closes EasyDynamics/oscal-react-library#570
laurelmay pushed a commit to EasyDynamics/oscal-rest that referenced this issue Apr 27, 2023
To stop unnecessary failures on prs from forks, this removes the triage
workflow

closes EasyDynamics/oscal-react-library#570
laurelmay pushed a commit to EasyDynamics/oscal-editor-deployment that referenced this issue Apr 27, 2023
To stop unnecessary failures on prs from forks, this removes the triage
workflow


closes EasyDynamics/oscal-react-library#570
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment