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

Post a Playground link on every PR using the pull_request_target workflow #5737

Closed

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Dec 7, 2023

Follows up on #5526

Related:

Updates the pull-request-comments.yml workflow to post the Playground comment on the pull_request_target trigger, not on the workflow_dispatch trigger.

Here's how it works in my fork: adamziel#11

The previous attempt leaned on workflow_dispatch. The goal was to only post a comment once the build workflow creates the wordpress.zip artifact required by the PR previewer. However, workflow_dispatch seems to have less permissions than the pull_request_target trigger. The workflows dispatched that way fail with a 403 error and the following error message:

data: {
      message: 'Resource not accessible by integration',
      documentation_url: 'https://docs.github.com/rest/actions/workflows#create-a-workflow-dispatch-event'
}

Since there doesn't seem to be a straightforward way to only post the comment when the build artifact is ready, I propose posting it immediately and then handling the UX on the PR previewer side. It could display a progress indicator and say "That Pull Request is still being built on GitHub, you'll be redirected to the preview as soon as it's ready!"

See how the commenting workflow failed on all these PRs:

CleanShot 2023-12-07 at 21 34 00@2x

Testing instructions

Security implications

The playground-details workflow follows the pattern established by the post-welcome-message workflow:

  • It's triggered by the pull_request_target trigger on the trunk branch.
  • It uses the same if condition and permissions clause.
  • It performs the same task of posting a comment using the GITHUB_TOKEN to access the repo.

Therefore, it shouldn't introduce any security risks.

Alternatives considered

Passing the GITHUB_TOKEN via inputs

The following dispatch call worked in my private fork, which made me hopeful. However, if I ran it without passing the github_token, it also worked. This is the how the original 403 error got merged via PR #5526. We would have to essentially merge this change and test it in production, which I'm not too excited about.

          script: |
            github.rest.actions.createWorkflowDispatch({
              owner: context.repo.owner,
              repo: context.repo.repo,
              workflow_id: 'pull-request-comments.yml',
              ref: 'trunk',
              inputs: {
                pr_number: '${{ github.event.number }}',
                github_token: '${{ secrets.GITHUB_TOKEN }}'
              }
            });

Moving the commenting action to the same workflow as the build jobs

The job responsible for commenting could be moved into test-build-processes.yml and run after the build workflows, avoiding this entire conundrum. However, a large point of splitting the jobs into multiple files, was to maintain the separation between the build jobs and the commenting jobs. Therefore, I propose to keep these separate.

Using on: workflow_run

@swissspidy proposed the following trigger in #5738:

on:
  workflow_run:
    workflows:
      - Test Build Processes
    types:
      - completed

Unfortunately, I could not get that to run on my fork. I suspect this might be related to this note in the workflows documentation:

When you use the repository's GITHUB_TOKEN to perform tasks, events triggered by the GITHUB_TOKEN, with the exception of workflow_dispatch and repository_dispatch, will not create a new workflow run.

https://docs.github.com/en/actions/using-workflows/triggering-a-workflow#triggering-a-workflow-from-a-workflow

Before merging

Let's do the reviewing and approving here, but let's hold off with merging until the related progress indicator work is shipped in the WordPress Playground repo.

The required changes in Playground were merged in WordPress/wordpress-playground#853

Trac Ticket: https://core.trac.wordpress.org/ticket/59416.

cc @desrosj @swissspidy

@adamziel adamziel changed the title Post a Playground link on every PR using the pull_request workflow Post a Playground link on every PR using the pull_request_target workflow Dec 10, 2023
@adamziel
Copy link
Contributor Author

cc @ockham

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

LGTM; let's hope it's gonna take 🤞

Thank you for also exploring the alternatives you mentioned!

@adamziel
Copy link
Contributor Author

Merged in https://core.trac.wordpress.org/changeset/57178

@adamziel adamziel closed this Dec 10, 2023
adamziel added a commit to adamziel/wordpress-develop that referenced this pull request Dec 10, 2023
This PR exists to test the user experience of previewing WordPress Pull
Requests.

See WordPress#5737

Ticket: 59416
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