Opened 13 months ago
Closed 3 months ago
#59416 closed task (blessed) (fixed)
Add a GitHub Action which alerts contributors to a WordPress Playground link to use for testing PRs
Reported by: | JeffPaul | Owned by: | |
---|---|---|---|
Milestone: | 6.6 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | has-patch |
Focuses: | Cc: |
Description
The following approach could help bring in many new folks to help with testing PRs as they no longer need to have a local set up and able to load patches/PRs locally and can quickly jump into a WordPress Playground environment to help test.
Gutenberg has a GitHub Action that builds a ZIP of the plugin (workflow file, example Gutenberg PR with built ZIP attached) which then WordPress Playground makes use of in the Gutenberg PR Previewer.
Taking that above functionality a step further and integrating for Core:
- Add a GitHub Action for wordpress-develop that builds a ZIP of core for a PR (similar to the Gutenberg action).
- Have that same action also add a comment to the PR with a link to a WordPress Playground that's built using the ZIP from the PR. So a comment like:
Thanks <@PR-author>! Here's a link to a site created from this PR that can be used for testing: <wordpress-playground-link>.
In my quick review of the WordPress Playground documentation it does reference an ability to pull requests from your repository, but I think that's crafted more for plugin developers and not for WordPress core itself. I took a look at ReplaceSiteStep, but I think that's just for loading in alternate content and not a full on separate WordPress ZIP. So, this functionality might require additional functionality within WordPress Playground but I'll try and chase down folks from that team to comment here on the topic.
Change History (94)
#2
@
13 months ago
Great idea! Thanks for sharing all the links.
I'd love to help make this happen. I'll investigate if Blueprints are enough to accomplish what we want.
#3
@
12 months ago
Note that https://github.com/WordPress/wordpress-develop/pull/5481 and https://github.com/WordPress/wordpress-playground/pull/700 help advance this topic. Review/feedback/help on those PRs would be wonderful for anyone interested.
This ticket was mentioned in PR #5481 on WordPress/wordpress-develop by @zieladam.
12 months ago
#4
- Keywords has-patch added
Creates a new GitHub workflow to expose WordPress build as a GitHub artifact. The goal is to enable previewing Pull Requests in WordPress Playground.
Related Playground PR https://github.com/WordPress/wordpress-playground/pull/700
Trac Ticket: https://core.trac.wordpress.org/ticket/59416
cc @gziolo @youknowriad @peterwilsoncc @desrosj
@Bernhard Reiter commented on PR #5481:
12 months ago
#5
And I just added a
zip -r build.zip build
line that sped up the upload step from 10 minutes to 19s. Woah!
That's due to a somewhat familiar issue with the upload-artifact
action: Uploading multiple files takes significantly more time than uploading a single file 😕
One thing to keep in mind here is that this results in the zip file that is uploaded as an artifact being zipped again. It's not immediately apparent, but it may require an additional step when using the artifact.
FWIW, the same tradeoff applies to Gutenberg (where it has confused users that uploaded the "double-zipped" gutenberg-plugin.zip
to their WP, expecting it to work like a "regular" plugin zip file); there's also an issue in the `upload-artifact` repo about the double-zip. We even attempted removing that extraneous zipping at some point (https://github.com/WordPress/gutenberg/pull/32780) but concluded that it wasn't worth the negative performance impact.
@zieladam commented on PR #5481:
12 months ago
#6
Shall we wait for 6.4 to be branched from trunk before landing this?
This change matters for trunk Pull Requests and even if the workflow file changes, I don't think we'll need to backport that. I'll go ahead and merge. Feel free to revert if I'm wrong here.
@zieladam commented on PR #5481:
12 months ago
#8
#9
@
12 months ago
I just shipped wordpress-develop PR previewer!
https://playground.wordpress.net/wordpress.html
You can even link directly to PRs:
https://playground.wordpress.net/wordpress.html?pr=5511&url=/wp-admin/post-new.php?test=1
Two notes:
- If your PR doesn’t work, rebase it. It needs to target the latest version of trunk with this commit in it.
- Downloading the built PR can take a while. Let’s find a way to optimize that!
Other than that, everything seems ripe to create a GitHub action which alerts contributors to a WordPress Playground link to use for testing PRs
#10
@
12 months ago
- Milestone changed from Awaiting Review to 6.4
To solve the double zip issue, what if the cd build && zip -r wordpress.zip .
step was replaced with rm -r $(ls -A | grep -v build ) && mv build/* . && rmdir build
which moves everything into the main folder?
I also updated the Milestone since this has a commit in 6.4.
#11
@
12 months ago
Thanks @jorbin! Double zipping is an unfortunate side-effect of how GitHub processes artifacts. If you upload a directory with multiple files, GitHub will process them one by one, take a long time to do it, and eventually zip everything into a single bundle. If you upload a zip file, it will be very fast but GitHub will zip it again anyway. It's not really an issue, just a small annoyance.
This ticket was mentioned in PR #5526 on WordPress/wordpress-develop by @zieladam.
12 months ago
#12
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: 59416
#13
@
12 months ago
I did a PR with a simple GitHub workflow to comment on every trunk PR with a link to a Playground-based preview:
https://github.com/WordPress/wordpress-develop/pull/5526
One issue with it is access – GitHub complains that workflow has no access rights to comment. I suppose we need an access token or some sort of repository setup to allow comment creation – any ideas how to address that @JeffPaul @peterwilsoncc @akirk?
@JeffPaul commented on PR #5526:
12 months ago
#14
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
@zieladam commented on PR #5526:
12 months ago
#16
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.
@zieladam commented on PR #5526:
12 months ago
#17
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 commented on PR #5526:
12 months ago
#18
@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.
@swissspidy commented on PR #5526:
12 months ago
#20
@desrosj correct me if I‘m wrong, but permissions are different for that trigger. See https://github.com/WordPress/wordpress-develop/pull/5526#discussion_r1370397098
12 months ago
#21
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
12 months ago
#22
Sorry for all the noise!
After some testing, I think you're right @swissspidy. I could have sworn that in the past I was able to get commenting to work within a PR, but I must be mixing that up either with workflows that are _triggered_ by other workflows within a PR (and thus run within the context of the repo itself) or instances where I had a PR from a branch within the same repository and not a fork.
I still feel like we should not have a separate workflow just for commenting. The commenting is related to the build step, and even depends on it (we shouldn't comment unless we know that a build has successfully been created). I've gone and combined the two, and also renamed build.yml
to wordpress.playground.yml
, which will allow us to include other additional logic related to Playground in the future.
A few other notes about the changes I included:
- I pinned a specific commit SHA to the version of actions/github-scripts. This is a recommended best practice in GitHub's hardening recommendations when using third-party actions.
- 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.
- I removed the
opened
type filter for the event as well. This ensures that the build runs on every update to the PR. - To prevent comments being posted every time the workflow runs, I've changed the commenting job to look for a preexisting comment before leaving a new one. This also solves the edge case where the first build attempt for a PR fails. The comment job will skip the first time, and it will run once there is a successful build.
- I expanded the comment a bit to call out some of the limitations and "gotchas" about using a Playground instance. I also linked off to the Limitations page in the documentation so someone can dig deeper if they would like.
One other thought. I'm not sure that running on workflow_dispatch
is useful if a Playground instance cannot be created using a commit hash. Something like https://playground.wordpress.net/wordpress.html?sha=abcdefghijklmnop
. Even if that's possible, I think that adding a push
trigger event would be better. That would ensure every commit to trunk
(or whatever branches we specify) has a built artifact that could be used in a Playground instance.
12 months ago
#23
Also, here is an example of what the content of the comment will look like:
## Test using WordPress Playground
The changes in this pull request can previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.
### Some things to be aware of
- The Plugin and Theme Directories cannot be accessed within Playground.
- All changes will be lost when closing a tab with a Playground instance.
- All changes will be lost when refreshing the page.
- A fresh instance is created each time the link below is clicked.
- Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance, it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.
@swissspidy commented on PR #5526:
12 months ago
#24
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: https://github.com/swissspidy/wordpress-develop/pull/33
@zieladam commented on PR #5526:
12 months ago
#25
I'm on a meetup this week and AFK the next one – feel absolutely free to take over here
#26
@
11 months ago
- Milestone changed from 6.4 to 6.5
Moving to milestone 6.5 as WP 6.4 RC3 has been released.
@zieladam commented on PR #5526:
11 months ago
#27
@desrosj did you manage to get it to work in your fork?
11 months ago
#28
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!
11 months ago
#29
OK, I think I've cleaned up my work and have it ready to go. You can see a test comment in desrosj/wordpress-develop#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.
@zieladam commented on PR #5526:
11 months ago
#30
@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!
11 months ago
#32
Merged in https://core.trac.wordpress.org/changeset/57124. However, it seems to be having issues dispatching the workflow: https://github.com/WordPress/wordpress-develop/actions/runs/6907809373/job/18797346079#step:2:82.
@zieladam commented on PR #5526:
11 months ago
#34
@desrosj weird! 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.
@zieladam commented on PR #5526:
11 months ago
#35
@desrosj interestingly, the "first contributor" comment still works: https://github.com/WordPress/wordpress-develop/pull/5685
@zieladam commented on PR #5526:
11 months ago
#36
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"?
@zieladam commented on PR #5526:
11 months ago
#37
This ticket was mentioned in PR #5737 on WordPress/wordpress-develop by @zieladam.
10 months ago
#38
Follows up on https://github.com/WordPress/wordpress-develop/pull/5526
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: https://github.com/adamziel/wordpress-develop/pull/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:
## Testing instructions
- Confirm the Playground comment is intact in this PR on Adam's fork: https://github.com/adamziel/wordpress-develop/pull/11
- Confirm the change looks good and makes sense
- Hope for the best since we can't test in the wordpress-develop repo without merging
Trac Ticket: https://core.trac.wordpress.org/ticket/59416.
cc @desrosj @swissspidy
This ticket was mentioned in PR #5742 on WordPress/wordpress-develop by @zieladam.
10 months ago
#39
Moves the zipping step before cleaning the dist directory.
The callable-test-core-build-process.yml zips the WordPress dist directory *after* npm run grunt clean
runs. This produces a mostly empty zip artifact that's only 332 bytes.
@zieladam commented on PR #5742:
10 months ago
#41
Thank you! Merged in https://core.trac.wordpress.org/changeset/57174.
@swissspidy commented on PR #5742:
10 months ago
#42
Note that you'll need to update the order of the comments above as well.
@zieladam commented on PR #5737:
10 months ago
#44
cc @ockham
@zieladam commented on PR #5737:
10 months ago
#46
This ticket was mentioned in PR #5752 on WordPress/wordpress-develop by @zieladam.
10 months ago
#47
This PR exists to test the user experience of previewing WordPress Pull Requests.
See https://github.com/WordPress/wordpress-develop/pull/5737
@zieladam commented on PR #5752:
10 months ago
#48
Let's keep this PR open until 20.12 as a live demo of the previewer. In the meantime, other PRs will get created and rebased so there will be ample examples of the previewer.
#49
@
10 months ago
I've merged https://github.com/WordPress/wordpress-develop/pull/5737 that runs the PR commenting workflow using the same mechanism as the "First contribution" workflow.
This fixes the CI checks for wordpress-develop PRs that failed due to the 403 error encountered by the previous version of that workflow.
Caveat: The comment with the preview link is now posted before the build artifact is ready, which could cause confusion if it led the user to an error page. This is why I also adjusted the user experience on WordPress Playground side of things to communicate how the build artifact is not ready yet, and that the previewer will auto-retry every 30 seconds.
Here's a PR I prepared to showcase how it works:
https://github.com/WordPress/wordpress-develop/pull/5752
Let's let this ticket sit for a bit to and gather feedback before closing.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
10 months ago
#51
@
10 months ago
A nice improvement would be to only post the preview link if the PR updated the src directory. It might not make much sense to preview PRs adjusting, say, GitHub workflows.
Also, noting here that @swissspidy explored using the workflow_run trigger to only post the comment once the build artifact is available to download: https://github.com/swissspidy/wordpress-develop/pull/55
#52
@
10 months ago
I recently tried out this feature and successfully tested a core ticket https://core.trac.wordpress.org/ticket/58498#comment:9 using the Playground.
It's truly amazing and has proven to be an excellent tool so far. The ease of use and efficiency make it incredibly helpful, saving a significant amount of time in the testing process.
Thanks, everyone for this great help!
@zieladam commented on PR #5526:
10 months ago
#54
This was merged in https://github.com/WordPress/wordpress-develop/pull/5737
#55
follow-up:
↓ 56
@
9 months ago
@zieladam @desrosj For the PR comments, what do you think about running the install with WP_DEBUG
set to true
so debug messages show?
For example instead of linking to https://playground.wordpress.net/wordpress.html?pr=5871 the link would become https://playground.wordpress.net/wordpress.html?pr=5871#%7B%22$schema%22:%22https://playground.wordpress.net/blueprint-schema.json%22,%22steps%22:%5B%7B%22step%22:%22defineWpConfigConsts%22,%22consts%22:%7B%22WP_DEBUG%22:true%7D%7D%5D%7D
We could include two links if you'd prefer to give people a choice.
#56
in reply to:
↑ 55
@
9 months ago
Replying to peterwilsoncc:
@zieladam @desrosj For the PR comments, what do you think about running the install with
WP_DEBUG
set totrue
so debug messages show?
I think that providing a few links to specific configurations is a great idea.
Another thing that has been on my mind is that the comment should only be left when the following things are changed:
- The
src
directory - The
(package|package-lock).json
files Gruntfile.js
tools/webpack
webpack.config.js
If things like GitHub workflows or test files are the only thing changed, then there's no reason to confuse a contributor since they won't actually be ble to test within the tool.
#57
@
8 months ago
This could probably be done by analyzing the file changes in the test-build-processes.yml
workflow and not upload the pr-number
artifact if it's not worthwhile. Then in pull-request-comments.yml
we can just silently bail if there's no artifact (rather than calling core.setFailed( 'No artifact found!' );
and causing the step to fail)
#59
@
8 months ago
Another thing that has been on my mind is that the comment should only be left when the following things are changed
I'd suggest opening a new ticket for that & closing this one. WDYT?
#61
@
6 months ago
Hey so when the GH Action adds the comment to a wordpress-develop PR with the WP Playground testing link, could it also add a similar comment into the respective Trac ticket (assuming a Trac ticket is linked to the GH PR)?
Perhaps even adding a View Playground
button next to the View patch
and View PR
buttons in the Pull Requests
section near the top of the Trac ticket?
I realized that anyone following along in only Trac won't necessarily see that Playground comment and link and could be missing an opportunity to help with testing.
This ticket was mentioned in PR #6738 on WordPress/wordpress-develop by @desrosj.
4 months ago
#68
There are times where the ZIP file of the build should be stored as an artifact but the PR number is not required.
For example, Playground testing information is not required for push events, only PRs. But the build ZIP is required for all events where the performance testing workflow runs (a comparison is made between the previous commit and the current one).
Trac ticket: https://core.trac.wordpress.org/ticket/59416
This sounds great, @JeffPaul.
One reason that the Playground defaults to official WordPress images is because it takes some work to strip out content that isn't needed and prevent downloading a large initial image to boot up. If you wanted to build a step to load a PR though I think that would be awesome. People wanting to test PRs against
wordpress-develop
would understand that there could be more initial download, and they are likely to have higher-than-average download speeds.Let me know how I can help; I'm mostly listed to one day a week on the Playground and am not the best guide, but I can try.