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 preview link on every PR #5526

Closed
wants to merge 43 commits into from

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Oct 18, 2023

Very simple GitHub workflow to comment on every trunk PR with a link to a Playground-based preview.

Other than access, it seems to be fine. The last thing to solve is the 403 error:

403 Resource not accessible by integration

https://github.com/WordPress/wordpress-develop/actions/runs/6572036562/job/17852349020?pr=5526

cc @jeffpaul

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

@jeffpaul
Copy link
Member

This action adds a comment, so perhaps there's something to glean from that to leverage here? https://github.com/WordPress/wordpress-develop/blob/trunk/.github/workflows/welcome-new-contributors.yml

cc: @desrosj

Copy link
Contributor

@desrosj desrosj left a comment

Choose a reason for hiding this comment

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

I don't think we should have a separate workflow for this. I think that this should be a step within the Build WordPress workflow after the artifact is built and uploaded.

.github/workflows/preview-comment.yml Outdated Show resolved Hide resolved
.github/workflows/preview-comment.yml Outdated Show resolved Hide resolved
@adamziel adamziel marked this pull request as draft October 24, 2023 20:50
@adamziel adamziel marked this pull request as ready for review October 24, 2023 20:50
@adamziel
Copy link
Contributor Author

Thanks you @desrosj @swissspidy! I just addressed the feedback on this PR, although we won't able to test it without merging. GitHub actions docs says:

This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does. This prevents execution of unsafe code from the head of the pull request that could alter your repository or steal any secrets you use in your workflow. This event allows your workflow to do things like label or comment on pull requests from forks. Avoid using this event if you need to build or run code from the pull request.

@adamziel
Copy link
Contributor Author

I don't think we should have a separate workflow for this. I think that this should be a step within the Build WordPress workflow after the artifact is built and uploaded.

This workflow now uses a different trigger (pull_request_target) than the Build WordPress workflow so it may need to remain separate.

@swissspidy
Copy link
Member

@adamziel you can test this by opening a PR on your own fork instead this repository. This way we can refine it before merging.

We can also try using a different trigger with a custom access token if we want to merge the workflow files.

@desrosj
Copy link
Contributor

desrosj commented Oct 24, 2023

What was the reason pull request wouldn't work? I think I'm missing that.

@swissspidy
Copy link
Member

@desrosj correct me if I‘m wrong, but permissions are different for that trigger. See #5526 (comment)

@desrosj
Copy link
Contributor

desrosj commented Oct 24, 2023

Ah, sorry I missed that because it was collapsed in the resolved discussion. We should be able to use permissions to fine tune the capability we need the workflow, job, or step to have. https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs#overview

@swissspidy
Copy link
Member

swissspidy commented Oct 25, 2023

I've made the commenting job dependent on the build one. We shouldn't comment on the PR until there actually is an artifact that can be used. This logic is untested though since we can't verify until merged.

And why can't we test this by opening a PR against the fork? That's how I successfully tested this in the past. This comment is created by the GitHub Action: swissspidy#33

@adamziel
Copy link
Contributor Author

I'm on a meetup this week and AFK the next one – feel absolutely free to take over here

@adamziel
Copy link
Contributor Author

adamziel commented Nov 6, 2023

@desrosj did you manage to get it to work in your fork?

@desrosj
Copy link
Contributor

desrosj commented Nov 6, 2023

I did, but I have a bit of cleanup to do. I plan to circle back to this later this week after 6.4 is out!

@desrosj
Copy link
Contributor

desrosj commented Nov 14, 2023

OK, I think I've cleaned up my work and have it ready to go. You can see a test comment in desrosj#141.

I chatted with @swissspidy, and thought about trying this way forward. We are already building WordPress in the workflow that tests the build process. So it makes sense to just perform the zipping and uploading of the artifact in that workflow instead of having a completely different one. That doesn't solve the issue with requiring pull_request_target though.

To solve this, I've consolidated the welcome comment workflow with the logic that comments with Playground details. It will run when a PR is opened on pull_request_target for the welcome message, but it will only post the comment with the Playground details after the build workflow has succeeded. This ensures that there is an artifact present that can actually be used. This is triggered using the workflow_dispatch event, which does not have the same issue as the pull_request event.

I don't love that it will run every time a PR is updated, but I'm not sure any way around that.

@adamziel
Copy link
Contributor Author

@desrosj I just reviewed and this looks good to me, thank you so much for taking the lead here! It indeed a pity this needs to run on every PR update, but I don't think it's a big deal. I like how you consolidated that with the new contributor comment 🎉 Let's ship it!

Copy link
Member

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

Let's give it a go!

@desrosj
Copy link
Contributor

desrosj commented Nov 17, 2023

@adamziel
Copy link
Contributor Author

adamziel commented Nov 18, 2023

@desrosj weird! I see the proper permissions are in place inside the workflow. Someone on StackOverflow suggested this:

go to https://github.com/OWNER/REPO/settings/actions and in Workflow Permissions section give actions Read and Write permissions. That provides your token with rights to modify your repo and solves your problem.

@adamziel
Copy link
Contributor Author

adamziel commented Nov 18, 2023

@desrosj interestingly, the "first contributor" comment still works. Also, dispatching the "Pull Request Comments" workflow manually seems to have worked – I attribute that to the workflow_dispatch condition but I'll try a few more things.

See my test PR: #5685.

@adamziel
Copy link
Contributor Author

So workflow_dispatch doesn't come with the same note about permissions as pull_request_target:

For workflows that are triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission

What if we just lean on pull_request_target and add a message on WordPress Playground side like "this Pull Request is still being built – please wait a few moments"?

@adamziel
Copy link
Contributor Author

I found one more issue with the uploaded artifact – it's 332 bytes and doesn't actually contain the built files:

CleanShot 2023-11-25 at 11 46 32@2x

@adamziel
Copy link
Contributor Author

This was merged in #5737

@adamziel adamziel closed this Dec 28, 2023
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.

4 participants