[infra] Integrate with external PR preview system

[jugglinmike]

Apologies for all the text. This is fairly involved, and I want to make sure everyone's on the same page. My main questions are

• Should we use the same GitHub-provided API access token used by the other Actions? If so, should we collapse this into a single long-running Action?
• Should we manage git refs using the git repository instead of the GitHub API?
• Should we ignore pull requests authored by downstream "export bots"?

I'm putting off writing automated tests until we iron out some of these details.

The pull request mirroring system is not yet deployed to https://wptpr.live. This shouldn't be merged before it is.

Commit message and design considerations follow.

---

Introduce a GitHub Action to monitor Pull Requests, storing relevant
information in the project's git repository (thus allowing the external
wptpr.live system to publish previews) and creating GitHub Deployments
(thus alerting contributors to the status of the preview). This Action
is triggered on a regular interval.

Introduce a second GitHub Action to monitor the state of the preview
system and communicate the relevant status to contributors via the Pull
Request UI. This Action is triggered for every GitHub Deployment created
in the previously-described Action.

For example, if three Pull Requests are updated, the first GitHub Action
will inspect them all. It will create GitHub Deployments only for the
"trusted" Pull Requests. A new GitHub Action will run for each of the
Deployments, polling the preview website until either the preview is
available or a timeout is reached. This Action will update the
deployment accordingly so that the author of each Pull Request author is
aware of the status of the preview site.

The following flow chart visually describes the same sequence:

                       sync
gh-101 (trusted) --->   |
gh-102 (untrusted) ->   |
gh-103 (trusted) --->   |
                      .----.
                      |sync|--------+---------------------.
                      '----'        |                     |
                             .-------------.       .-------------.
                             |deploy gh-101|       |deploy gh-103|
                             '-------------'       '-------------'
                                    |                     |
                             poll for preview      poll for preview
                                    |                     |
gh-101 <------ success ----- preview available     poll for preview
                                                          |
gh-103 <------- error --------------------------------- timeout

---

Here's a demonstration of the basic behavior (intentionally using the account
of a non-collaborator) in the wpt-actions-test repository:

web-platform-tests/wpt-actions-test#20

The GITHUB_TOKEN Secret is the most direct way to grant "write" privileges to Actions. Unfortunately, this script currently needs to trigger Actions from another Action, and that's not possible with the GITHUB_TOKEN. There are two ways around this:

1. Use a user-generated token; benefits: easier to communicate state, easier to test, reduced risk of interference with other services due to API quota enforcement
2. Refactor to do all the work (creating one or more Deployments and polling for previews) from the same Action; benefits: does not require maintenance of another secret

For now, I've opted for the first approach (that's why the @wpt-pr-bot requested the Deployment in the demonstration above). The API request quota safeguard is less important if we're using an isolated access token, so we might want to simplify and remove that code if we stick with this approach.

The script creates and updates git refs using the GitHub API. If we wanted to reduce usage of the API (to avoid reaching the quota), we could perform this via git, but we'd have to then pull each preview-enabled branch.

Finally, I'm wondering if we should provide previews for Pull Requests opened by bots.

[foolip]

I'd like to not use a label here. When it was used, it cluttered up any PR listing quite badly, and when we're using deployments instead it ought not be needed.

[jugglinmike]

The GitHub Issue Label isn't a vestige from the prior implementation; it's the user-facing mechanism for enabling previews from untrusted forks. That's important even in a solution that uses GitHub Deployments. To me, it's an improvement over the comment-based approach offered by the w3c-test.org implementation.

We could use the label only for that purpose, so that only humans apply it. One drawback is project collaborators won't be able to opt out of the automatic preview (under the solution proposed here, they would do this by removing the automatically-assigned Label). Opting out is a feature of the w3c-test.org implementation, but I doubt it's a highly desired one, so maybe that's acceptable.

If we do limit the usage in this way, I'd like to change the name to "safelisted-for-preview".

Currently, the wpt.fyi system publishes previews for all refs that exist in two namespaces: refs/prs-open/* and refs/prs-labeled-for-preview/*. With this change, we'll certainly want to rework that. The simplest would be to rename the latter to refs/prs-trusted-for-preview/*, though that promotes the concept of "trust" in a way that couples the two projects. It would be more hygienic to work from three namespaces: refs/prs-open/*, refs/prs-safelisted/*, and refs/prs-from-collaborators/*.

So while removing automated application of the Label is fairly straightforward, it has ramifications that require a few hours of work. I'll hold off on that until there's agreement on the topic.

[foolip]

A "safelisted-for-preview" label that's only used to trigger mirroring where it otherwise wouldn't happen sounds good to me.

Since the refs are just to communicate to wptpr.live what it should do and it doesn't need to know why a PR should be deployed, won't two sets of refs still suffice? Just rename refs/prs-labeled-for-preview/* to refs/prs-deployable/* or something?

[jugglinmike]

Since the refs are just to communicate to wptpr.live what it should do and it doesn't need to know why a PR should be deployed, won't two sets of refs still suffice?

That's true, and going further, we only technically need one ref. Something like refs/prs-open-and-trusted/* would be sufficient for wptpr.live to do its job.

Great minds think alike! That's how I originally designed the prior iteration of this task. @jgraham pushed back on that, writing

it highly couples things to the exact requirements of live preview; we only get tags on items that are both open PRs and also have the right label set. We could decouple those things and create refs for open PRs and refs for things labelled for preview.

I agreed, which is why this iteration started with two ref namespaces. My suggestion to introduce a third namespace is an application of the same rationale.

Then again, while a distinct prs-open ref namespace might be useful elsewhere, I'm less confident that a prs-from-collaborators would be (and even less so about prs-safelisted). I don't want to increase complexity to reduce coupling if there's no practical benefit. @jgraham do you have an opinion?

[jgraham]

Not a strong opinion, but I think two is fine. "PRs that are open" seems like a clearly reusable thing, but trying to split the difference between "PRs that are trusted because they're from collaborators" and "PRs that are trusted because they were marked as such" seems like it might be overkill in terms of enabling other consumers.

[jugglinmike]

Thanks, @jgraham. I've updated this accordingly. To summarize: the GitHub Issue label in use is "safelisted-for-preview", and it is only referenced by this script (that is, never introduced) in order to determine when a submission from a non-Collaborator should be mirrored. The underlying ref namespace is refs/prs-trusted-for-preview/*.

...I think. This is arduous to verify because the patch still lacks automated tests. Now that three members of the "wpt core team" have participated in this discussion, and no one has objected, I think it's prudent to write some tests. I'll plan to get started on that tomorrow.

[foolip]

Should we use the same GitHub-provided API access token used by the other Actions? If so, should we collapse this into a single long-running Action?

Using DEPLOY_TOKEN as you've done seems OK. That's already used in documentation.yml, is it already a @wpt-pr-bot token?

Looks like the current setup on polling vs. long-running is that the polling action triggers the long-running ones. Are you thinking of collapsing into the polling actions sometimes waiting around? If this is possible without creating a race condition between two actions trying to update the same deployment status, that sounds good.

Should we manage git refs using the git repository instead of the GitHub API?

create_ref is using the API now. Would there be benefits to using the git CLI instead?

Should we ignore pull requests authored by downstream "export bots"?

Yeah, let's do that for now.

[jugglinmike]

Using DEPLOY_TOKEN as you've done seems OK. That's already used in documentation.yml, is it already a @wpt-pr-bot token?

Yup, but don't take my word for it. The proof is in the history of the gh-pages branch, where the @wpt-pr-bot is the author of every commit (well, almost every commit).

Looks like the current setup on polling vs. long-running is that the polling action triggers the long-running ones. Are you thinking of collapsing into the polling actions sometimes waiting around? If this is possible without creating a race condition between two actions trying to update the same deployment status, that sounds good.

Deployments are associated with commits. Since the deployments will be created immediately in either approach, neither is more susceptible to race conditions.

create_ref is using the API now. Would there be benefits to using the git CLI instead?

This is also true of update_ref. If we instead shelled out to the git CLI, then we would reduce the impact that this script has on the API request quota.

Should we ignore pull requests authored by downstream "export bots"?

Yeah, let's do that for now.

Done

[zcorpan]

I removed the .github label per web-platform-tests/wpt-pr-bot#105

Let's see if a new label appears... Edit: it did!

[jugglinmike]

Okay, tests are up. Also, support for Python 3--tox is great!

[foolip]

@jugglinmike there are still CI failures, can you take a look?

[foolip]

@stephenmcgruer @Hexcles would either of you be up for reviewing this?

[stephenmcgruer]

Happy to review, but at BlinkOn until next week. Wouldn't be able to look at this till Monday, sorry.

[stephenmcgruer]

Generally LGTM. A few comments/questions, but nothing blocking I think.

[jugglinmike]

Thanks for the review, @stephenmcgruer! Unfortunately, unrelated infrastructure failures keep getting in the way of the validation for this patch. I've pushed up another empty commit; hopefully, the third time's the charm (though I won't be around to see until tomorrow).

[stephenmcgruer]

Dammit, I totally missed "The pull request mirroring system is not yet deployed to https://wptpr.live. This shouldn't be merged before it is.".

@jugglinmike Unlikely you're still awake, but is the PR mirroring system deployed to wptpr.live yet?

[Hexcles]

It has been deployed, but looks like Mike has not verified it end to end?

[stephenmcgruer]

Well... this is one way to verify it end to end. I'm intrigued, how about we let this new action run at least once and see what happens?

[jugglinmike]

Robert's right on both counts.

Something has certainly happened. http://wptpr.live currently offers a slew of unexpected html documents at the top level. Investigating presently

[stephenmcgruer]

Also:

.github/workflows/pull_request_previews.yml
Invalid Workflow File

DETAILS
yaml: line 4: did not find expected alphabetic or numeric character

[stephenmcgruer]

Btw, Robert and I looked at http://wptpr.live/ the other day (before this) and it looked much like it looks currently (in terms of those unexpected documents)

[jugglinmike]

Btw, Robert and I looked at http://wptpr.live/ the other day (before this) and it looked much like it looks currently (in terms of those unexpected documents)

Yeah, given the nature of the problem being reported by GitHub, those files couldn't have been created in response to the merging of this pull request.

Here's a fix for the syntax error: #20347

[stephenmcgruer]

With the syntax error fixed, we have had a pr-preview action trigger successfully. It hit https://api.github.com/search/issues?q=repo:web-platform-tests/wpt+is:pr+updated:>2019-11-20T20:03:09Z and found 0 pull requests.

[jugglinmike]

The subsequent run demonstrates slightly more of the intended behavior:

./tools/ci/pr_preview.py --host https://api.github.com --github-project web-platform-tests/wpt synchronize --window 480
INFO:__main__:Issuing request: GET https://api.github.com/rate_limit
INFO:__main__:Response status code: 200
INFO:__main__:Limit for "search" resource: 30/30
INFO:__main__:Searching for Pull Requests updated since 2019-11-20T20:09:07Z
INFO:__main__:Issuing request: GET https://api.github.com/search/issues?q=repo:web-platform-tests/wpt+is:pr+updated:>2019-11-20T20:09:07Z
INFO:__main__:Response status code: 200
INFO:__main__:Found 2 Pull Requests
INFO:__main__:Processing Pull Request #19838
INFO:__main__:Pull Request should not be mirrored
INFO:__main__:Processing Pull Request #19632
INFO:__main__:Pull Request should not be mirrored

Pull request number 19838 is closed, and pull request number 19632 was opened by a non-collaborator

[stephenmcgruer]

That issue is a bit hairy, so I'm waiting for a 'normal' PR to drop a safe for preview label on (or lmk if you don't want that done yet)

[stephenmcgruer]

Ah, I see you deliberately triggered it on PR 20239. Fingers crossed!

[stephenmcgruer]

http://wptpr.live/20239/ exists! \o/ EDIT: With the file added in that PR \o/

[jugglinmike]

The script successfully created a deployment, and the corresponding pull request was updated to describe an "in progress" deployment. wptpr.live recognized the new tags and mirrored as anticipated:

$ curl http://wptpr.live/.git/worktrees/20239/HEAD
c2cf7ba8a6730e847724d6d8c718ab4a2eafc0ce
$ curl --silent http://wptpr.live/20239/LICENSE.md | head -n1
# The 3-Clause BSD License

...but the corresponding pr-preview-detect workflow did not run. Becuase of that, the GitHub UI for the pull request still lists the deployment as "pending" and does not include a link to the preview.

I will continue to investigate and report back here.

[sideshowbarker]

Something has certainly happened. http://wptpr.live currently offers a slew of unexpected html documents at the top level. Investigating presently

That happens sometimes at http://w3c-test.org/ too. In fact that’s the same state it’s in right now. When it happens, I’ve just gone into the environment on the host and manually deleted the files. I’ve never been able to discern the cause (though I’ve not really ever seriously investigated it).