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

[infra] Integrate with external PR preview system #19838

Merged
merged 7 commits into from Nov 20, 2019

Conversation

@jugglinmike
Copy link
Contributor

jugglinmike commented Oct 22, 2019

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.

API_RATE_LIMIT_THRESHOLD = 0.2
# The GitHub Pull Request label which indicates that a Pull Request is expected
# to be actively mirrored by the preview server
LABEL = 'pull-request-has-preview'

This comment has been minimized.

@foolip

foolip Oct 23, 2019 Member

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.

This comment has been minimized.

@jugglinmike

jugglinmike Oct 23, 2019 Author Contributor

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.

This comment has been minimized.

@foolip

foolip Oct 24, 2019 Member

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?

This comment has been minimized.

@jugglinmike

jugglinmike Oct 25, 2019 Author Contributor

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?

This comment has been minimized.

@jgraham

jgraham Oct 28, 2019 Contributor

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.

This comment has been minimized.

@jugglinmike

jugglinmike Oct 28, 2019 Author Contributor

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
Copy link
Member

foolip commented Oct 23, 2019

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
Copy link
Contributor Author

jugglinmike commented Oct 23, 2019

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

foolip added a commit that referenced this pull request Oct 24, 2019
jugglinmike added a commit that referenced this pull request Oct 25, 2019
@zcorpan zcorpan removed the .github label Oct 25, 2019
@zcorpan
Copy link
Member

zcorpan commented Oct 25, 2019

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
Copy link
Contributor Author

jugglinmike commented Nov 1, 2019

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

@foolip
Copy link
Member

foolip commented Nov 6, 2019

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

@foolip
Copy link
Member

foolip commented Nov 14, 2019

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

@stephenmcgruer
Copy link
Contributor

stephenmcgruer commented Nov 14, 2019

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

Copy link
Contributor

stephenmcgruer left a comment

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

tools/ci/pr_preview.py Show resolved Hide resolved
tools/ci/pr_preview.py Outdated Show resolved Hide resolved
tools/ci/pr_preview.py Outdated Show resolved Hide resolved
tools/ci/pr_preview.py Show resolved Hide resolved
@jugglinmike
Copy link
Contributor Author

jugglinmike commented Nov 20, 2019

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).

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 20, 2019
…kflow definitions, a=testonly

Automatic update from web-platform-tests
[GitHub Actions] put `name` first in workflow definitions (#19871)

Matching web-platform-tests/wpt#19838.
--

wpt-commits: 82311342f330af3a586d76e2b4bec8056fb87ca5
wpt-pr: 19871

Differential Revision: https://phabricator.services.mozilla.com/D53458
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Nov 20, 2019
…kflow definitions, a=testonly

Automatic update from web-platform-tests
[GitHub Actions] put `name` first in workflow definitions (#19871)

Matching web-platform-tests/wpt#19838.
--

wpt-commits: 82311342f330af3a586d76e2b4bec8056fb87ca5
wpt-pr: 19871

Differential Revision: https://phabricator.services.mozilla.com/D53458
@stephenmcgruer stephenmcgruer merged commit 990bd8b into web-platform-tests:master Nov 20, 2019
23 checks passed
23 checks passed
build-and-publish
Details
build-and-tag
Details
Azure Pipelines #20191120.10 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (infrastructure/ tests: macOS) infrastructure/ tests: macOS succeeded
Details
Azure Pipelines (tools/ unittests: Windows Python 3) tools/ unittests: Windows Python 3 succeeded
Details
Azure Pipelines (tools/ unittests: Windows) tools/ unittests: Windows succeeded
Details
Azure Pipelines (tools/ unittests: macOS) tools/ unittests: macOS succeeded
Details
Azure Pipelines (tools/wpt/ tests: Windows) tools/wpt/ tests: Windows succeeded
Details
Azure Pipelines (tools/wpt/ tests: macOS) tools/wpt/ tests: macOS succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: Windows) tools/wptrunner/ unittests: Windows succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: macOS) tools/wptrunner/ unittests: macOS succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Community-TC (pull_request) TaskGroup: success
Details
staging.wpt.fyi - chrome[experimental] Chrome results
Details
staging.wpt.fyi - firefox[experimental] Firefox results
Details
staging.wpt.fyi - safari[experimental] Safari results
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@stephenmcgruer
Copy link
Contributor

stephenmcgruer commented Nov 20, 2019

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
Copy link
Member

Hexcles commented Nov 20, 2019

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

@stephenmcgruer
Copy link
Contributor

stephenmcgruer commented Nov 20, 2019

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
Copy link
Contributor Author

jugglinmike commented Nov 20, 2019

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
Copy link
Contributor

stephenmcgruer commented Nov 20, 2019

Also:

.github/workflows/pull_request_previews.yml
Invalid Workflow File

DETAILS
yaml: line 4: did not find expected alphabetic or numeric character
@stephenmcgruer
Copy link
Contributor

stephenmcgruer commented Nov 20, 2019

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
Copy link
Contributor Author

jugglinmike commented Nov 20, 2019

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
Copy link
Contributor

stephenmcgruer commented Nov 20, 2019

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
Copy link
Contributor Author

jugglinmike commented Nov 20, 2019

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
Copy link
Contributor

stephenmcgruer commented Nov 20, 2019

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
Copy link
Contributor

stephenmcgruer commented Nov 20, 2019

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

@stephenmcgruer
Copy link
Contributor

stephenmcgruer commented Nov 20, 2019

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

@jugglinmike
Copy link
Contributor Author

jugglinmike commented Nov 20, 2019

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.

gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Nov 21, 2019
…kflow definitions, a=testonly

Automatic update from web-platform-tests
[GitHub Actions] put `name` first in workflow definitions (#19871)

Matching web-platform-tests/wpt#19838.
--

wpt-commits: 82311342f330af3a586d76e2b4bec8056fb87ca5
wpt-pr: 19871

Differential Revision: https://phabricator.services.mozilla.com/D53458

UltraBlame original commit: f81457e90eadaaf76e4de1a03698a98d05e57399
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Nov 21, 2019
…kflow definitions, a=testonly

Automatic update from web-platform-tests
[GitHub Actions] put `name` first in workflow definitions (#19871)

Matching web-platform-tests/wpt#19838.
--

wpt-commits: 82311342f330af3a586d76e2b4bec8056fb87ca5
wpt-pr: 19871

Differential Revision: https://phabricator.services.mozilla.com/D53458

UltraBlame original commit: f81457e90eadaaf76e4de1a03698a98d05e57399
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Nov 21, 2019
…kflow definitions, a=testonly

Automatic update from web-platform-tests
[GitHub Actions] put `name` first in workflow definitions (#19871)

Matching web-platform-tests/wpt#19838.
--

wpt-commits: 82311342f330af3a586d76e2b4bec8056fb87ca5
wpt-pr: 19871

Differential Revision: https://phabricator.services.mozilla.com/D53458

UltraBlame original commit: f81457e90eadaaf76e4de1a03698a98d05e57399
@sideshowbarker
Copy link
Member

sideshowbarker commented Dec 11, 2019

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.