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

Store WordPress "build" directory as an artifact to enable live previews with Playground #5481

Closed
wants to merge 18 commits into from

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Oct 13, 2023

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 WordPress/wordpress-playground#700

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

cc @gziolo @youknowriad @peterwilsoncc @desrosj

adamziel added a commit to WordPress/wordpress-playground that referenced this pull request Oct 13, 2023
This PR, paired with WordPress/wordpress-develop#5481, enables
previewing WordPress Pull Requests.

Changes:

* Adds wordpress.html – an easy entrypoint to previewing PRs
* Allows pulling GH artifacts from wordpress-develop repo
* Patches the controlled iframe in JavaScript, not in Docker, to enable
  re-applying the patch in wordpress-develop branch #668
* Serves static files using the PHP instance when they exist, not based
  on heuristics like path.startsWith('/wp-content')
@jeffpaul
Copy link
Member

Note that this is a foundational item for https://core.trac.wordpress.org/ticket/59416 and will hopefully make it dramatically easier for folks to help test PRs. Thanks for the work on this @adamziel. This is so great, I LOVE IT!!!

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'm a little confused as to why we're uploading the artifacts 3 times. Could you clarify?

Also, I'm wondering if there is a benefit to create a new workflow that's specifically meant to just build the package and upload it as an artifact. That would open the door to perform any other additional steps we may need down the road as well.

.github/workflows/end-to-end-tests.yml Outdated Show resolved Hide resolved
.github/workflows/end-to-end-tests.yml Outdated Show resolved Hide resolved
.github/workflows/end-to-end-tests.yml Outdated Show resolved Hide resolved
adamziel added a commit to WordPress/wordpress-playground that referenced this pull request Oct 16, 2023
Add wordpress.html, a simple HTML form that enables previewing
any WordPress core Pull Request.

Technically, it's very similar to gutenberg.html – it just creates a
tailored Blueprint and redirects to Playground with that Blueprint
applied.

*Caveats*
This leans on storing WordPress builds as a GitHub artifact in CI. For now only one PR does that – WordPress/wordpress-develop#5481. Once it's merged, the previewer proposed in this PR will work for every wordpress-develop Pull Request!

The downloaded WordPress bundle is ~45MB and takes a while to download without any visible progress information. Let's find a way to optimize it.
adamziel added a commit to WordPress/wordpress-playground that referenced this pull request Oct 16, 2023
Add wordpress.html, a simple HTML form that enables previewing
any WordPress core Pull Request.

Technically, it's very similar to gutenberg.html – it just creates a
tailored Blueprint and redirects to Playground with that Blueprint
applied.

*Caveats*
This leans on storing WordPress builds as a GitHub artifact in CI. For now only one PR does that – WordPress/wordpress-develop#5481. Once it's merged, the previewer proposed in this PR will work for every wordpress-develop Pull Request!

The downloaded WordPress bundle is ~45MB and takes a while to download without any visible progress information. Let's find a way to optimize it.
@adamziel adamziel force-pushed the upload-build-for-playground branch from 8c55316 to 3619be7 Compare October 16, 2023 13:07
@adamziel
Copy link
Contributor Author

I'm a little confused as to why we're uploading the artifacts 3 times. Could you clarify?

@desrosj that's what I meant by "it's overeager and tries to upload multiple times" in the description. No purpose in particular, I just ran out of time to fix it on Friday. I just corrected that so it only uploads the artifact once. I also applied the rest of your feedback, thank you!

Right now I'm trying to figure out why GitHub is not running the workflow after I moved it to a separate build.yml file.

@desrosj
Copy link
Contributor

desrosj commented Oct 16, 2023

Right now I'm trying to figure out why GitHub is not running the workflow after I moved it to a separate build.yml file.

Hmm, looking at the commit you made above, I'm not sure either. The paths under the pull_request event are probably unnecessary. We want each change to trigger a build. With those paths defined, we may not get an artifact every time.

Another thing I've been thinking about. Will these artifacts need to be available more widely? I think the current intent is to share a link within the context of a PR. But if we'd need to use the artifact anywhere else, I'm not sure if it's easy to retrieve the artifact. it may be worth looking at publishing these as a package on GitHub, or something similar. This may be a problem for a different day, though.

@adamziel
Copy link
Contributor Author

But if we'd need to use the artifact anywhere else, I'm not sure if it's easy to retrieve the artifact.

It's not easy and I'd love to make it easy at one point. At the same time, artifacts solve a lot of problems, like cleaning up old files. I'd say let's move forward with them for now and let's explore more accessible uploads separately.

@adamziel
Copy link
Contributor Author

adamziel commented Oct 16, 2023

Also, I found the problem with a separate workflow file. I forgot about the required runs-on option. I fixed it and the build workflow now works as expected. Example run.

@adamziel
Copy link
Contributor Author

And I just added a zip -r build.zip build line that sped up the upload step from 10 minutes to 19s. Woah!

Does this look good, or do you see any blockers @desrosj?

.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
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.

Sorry, a few more thinks that I noticed!

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@desrosj
Copy link
Contributor

desrosj commented Oct 16, 2023

Added a few small requests. Also, thinking about this more, if the intent is for this workflow to run on any branch, we will need to backport this and should wait until after WP 6.4 is branched to commit to avoid having to backport frequent changes to the 6.3 branch as we refine.

I think it's fair to say we should only run this for PRs to and commits to trunk for now. Added a comment inline about that.

@desrosj
Copy link
Contributor

desrosj commented Oct 16, 2023

And I just added a zip -r build.zip build line that sped up the upload step from 10 minutes to 19s. Woah!

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.

@adamziel
Copy link
Contributor Author

@desrosj good feedback, thank you! I just addressed all your notes.

I think it's fair to say we should only run this for PRs to and commits to trunk for now. Added a comment inline about that.

Yes, I only want to run this for PRs so we're good.

One thing to keep in mind here is that this results in the zip file that is uploaded as an artifact being zipped again.

This is fine, Playground can handle that – the Gutenberg repo does the same thing.

@ockham
Copy link
Contributor

ockham commented Oct 17, 2023

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 (WordPress/gutenberg#32780) but concluded that it wasn't worth the negative performance impact.

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
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!

Shall we wait for 6.4 to be branched from trunk before landing this? 🤔 (That should happen shortly after RC1, which is scheduled for later tonight.)

@adamziel
Copy link
Contributor Author

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.

@adamziel
Copy link
Contributor Author

https://core.trac.wordpress.org/changeset/56958

@adamziel adamziel closed this Oct 17, 2023
adamziel added a commit to WordPress/wordpress-playground that referenced this pull request Oct 17, 2023
Add wordpress.html, a simple HTML form that enables previewing
any WordPress core Pull Request.

Technically, it's very similar to gutenberg.html – it just creates a
tailored Blueprint and redirects to Playground with that Blueprint
applied.

*Caveats*
This leans on storing WordPress builds as a GitHub artifact in CI. For now only one PR does that – WordPress/wordpress-develop#5481. Once it's merged, the previewer proposed in this PR will work for every wordpress-develop Pull Request!

The downloaded WordPress bundle is ~45MB and takes a while to download without any visible progress information. Let's find a way to optimize it.
adamziel added a commit to WordPress/wordpress-playground that referenced this pull request Oct 17, 2023
## What is this PR doing?

Support for previewing wordpress-develop Pull Requests.

<img width="1133" alt="CleanShot 2023-10-13 at 18 53 52@2x"
src="https://github.com/WordPress/wordpress-playground/assets/205419/e30e9ac6-c99e-4e81-b68f-45e14116f6c9">

The previewer lives in `/wordpress.html` and supports a few query
parameters:

* `?pr=5481` – preview that PR
* `?url=/wp-admin/post-new.php` – load that URL in WordPress
* `?mode=seamless` – redirect to a full-screen Playground without the
browser chrome and any other extra UI elements

## Caveats

The downloaded WordPress bundle is ~45MB and takes a while to download
without any visible progress information. Let's find a way to optimize
it.

## Testing instructions

1. Go to http://localhost:5400/website-server/wordpress.html
2. Preview PR 5481 (no other PR works yet, see
WordPress/wordpress-develop#5481)
3. Confirm in wp-admin the WordPress version is 6.4 dev

## Screenshots

<img width="1286" alt="CleanShot 2023-10-13 at 18 58 36@2x"
src="https://github.com/WordPress/wordpress-playground/assets/205419/ca25a714-4491-4f63-b82c-b2178a2828b5">

cc @spacedmonkey @fabiankaegy @dmsnell @jonathanbossenger @dawidurbanski
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