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

refactor(ci/pr-review): use workflow_call instead of workflow_run event (v2) #30064

Merged
merged 15 commits into from
Nov 6, 2023

Conversation

yin1999
Copy link
Member

@yin1999 yin1999 commented Nov 6, 2023

Description

This PR mainly switch to workflow_call instead of workflow_run event to prevent calling "PR review companion" when not necessary.

To achieve this, and take this opportunity to improve the code, the following changes were made:

  1. Use pull_request_target event instead of pull_request, with the former event, the GITHUB_TOKEN has write permissions and the jobs could access GitHub secrets (we are using secrets to store the AK, SK).
  2. Set the permissions of the "tests" job to read-all to avoid accidental writes to the repo.
  3. Run the "PR review companion" as a spread job (named "review") in "PR test", so we could use if condition to skip unnecessary runs,
    Set the permissions of "review" job to write-all, so the deployer could create comment in PRs.
    Add secrets: inherit to inherit the secrets from the parent workflow so the reusable workflow could access all secrets. (tested in: remove some lines for test yin1999/content#8, https://github.com/yin1999/content/actions/runs/6769815012/job/18397036827?pr=8#step:10:34)
  4. Use actions/download-artifact instead of js scripts, as the shared "PR review companion" workflow is a spread job in "PR test" now, we could use this action to download and unzip artifacts (simplifies the steps).

Security statement

Because the GITHUB_TOKEN generated in workflows that triggered by pull_request_target event has write permissions, we need to avoid malicious damage to the repo by the PR author.

  1. Set permissions to different jobs: as the first job would run js scripts which could be modified by PR authors, set the permission of this job to read-all (as we still need the GITHUB_TOKEN to access some resouces).
  2. The pull_request_target event would trigger the workflow runs with the main version of workflows (including the shared workflows - "PR review companion"). This has been tested by Onkar - Test workflow_call yin1999/content#6, and has been documented by GitHub Docs:

    Although the workflow runs in the context of the base of the pull request, you should make sure that you do not check out, build, or run untrusted code from the pull request with this event.

  3. The workflow triggered by pull_request_target event could also access GitHub secrets. But GitHub Actions can only read a secret if you explicitly include the secret in a workflow.

Related issues and pull requests

implementation of: #28579 (comment)
Previous attempt: #28617

@yin1999 yin1999 requested a review from a team as a code owner November 6, 2023 11:10
@github-actions github-actions bot added the system [PR only] Infrastructure and configuration for the project label Nov 6, 2023
@bsmth
Copy link
Member

bsmth commented Nov 6, 2023

Thanks for V2, Yin. I think we can merge later today keep an eye on CI again.

@bsmth bsmth self-assigned this Nov 6, 2023
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Also leaving a +1, will merge this one later this afternoon and monitor builds 🔬

@bsmth bsmth merged commit 9b1c805 into mdn:main Nov 6, 2023
@bsmth
Copy link
Member

bsmth commented Nov 6, 2023

@yin1999 @caugner you can see the new workflow in action here: https://github.com/mdn/content/actions/runs/6772741006?pr=28731

Well done 🙌🏻

@yin1999 yin1999 deleted the pr-review branch November 6, 2023 16:22
@yin1999
Copy link
Member Author

yin1999 commented Nov 6, 2023

Thanks for your help @bsmth, @caugner.

I would like to introduce it into translated-content in a few days, until then let's check if there is any other side effects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
system [PR only] Infrastructure and configuration for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants