-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Introduce automation to support pull rqst previews #17680
Conversation
Today, http://w3c-test.org automatically fetches and publishes the code from "preview-enabled" pull requests to WPT. It defines eligible pull requests as those authored by project collaborators and those where a collaborator has left a special GitHub comment. That mechanism has a number of drawbacks: - The configuration is hidden. Only those with admin rights can review the GitHub webhook upon which the solution relies. - Pull request state is opaque. It's not possible to know when/if a pull request has been recognized by w3c-test.org from the pull request or from WPT (though this information is available at the undocumented page http://w3c-test.org/submissions/). - Server state (i.e. the collection of which pull requests are considered "active") is likewise opaque. Also, in the absence of a backup mechanism, it is non-recoverable following a system crash - Previews cannot be disabled This patch is one component of an attempt to reimplement this service in a way that gives more control to the WPT administration and stores more state in the WPT repository. - Maintains state via git tags. This avoids the complexities of an external storage system (access credentials, back up policies, etc.) - Observes eligibility using GitHub pull request labels. This makes the state of the system apparent from the pull requests themselves. It also provides a more ergonomic and discoverable mechanism for enabling/disabling previews This is facilitated by a script that is executed as a GitHub Action. Although the script relies on a secret access token, it is *not* limited to use for pull requests from trusted collaborators due to the way GitHub Actions are executed [1] > ### Pull request events for forked repositories > > [...] > > #### Pull request with base and head branches in different repositories > > The base repository receives a `pull_request event` where the SHA is > the latest commit of base branch and ref is the base branch. With this in place, a far simpler "preview" server can be built. The simplified version will need no privileged information; it will only need to poll the git repository for tags and synchronize its deployment according to the result. [1] https://developer.github.com/actions/managing-workflows/workflow-configuration-options/#pull-request-events-for-forked-repositories
Add support for Python 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor copy edit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the code and found not problems — but I don’t know how to test it independently, so I’ll defer to others for approving it
So I think I like the general idea here. Some thoughts
|
Use generic git refs instead of tags
That would certainly simplify this code a bit, and although the preview server would have to be slightly more intelligent (i.e. finding the union of the ref sets), I think the system would be less complex overall. However, you're motivated by coupling, so I'd like to understand that perspective. Do you anticipate a need for something like this beyond the preview service? One of the benefits I had in mind about this was the ease with which collaborators could create a preview without a pull request--they could just push a ref. Come to think of it, this pull request ought to include documentation for that use case if we're going to support it, but the process will be more cumbersome if two refs are required. A two-ref approach also has an additional failure mode where the refs don't match.
Good idea. I just had to verify that fetching and pruning these refs would be possible. We'll need to use some of git's plumbing, but it seems doable.
We considered it briefly :P Switching to in-process mocks would eliminate less than 90 lines of code. I decided against it because I wanted to avoid coupling between the tests and the script. That's yet another kind of maintenance burden we ought to weigh. Process-level separation gives me more confidence that future contributors won't accidentally introduce tautologies resulting from bad mocks or global state. |
The list of open PRs seems like something that can be useful for several services e.g. browser sync services may need this information. Obviously you can just ask GitHub but the motivation for this kind of change seems to be to avoid having to access the GitHub API in more cases.
This is true.
It's the same system that GitHub uses to store PR refs, for example, so this is a well-worn path, not some mad thing I invented. |
If we create refs for everything which receives the GitHub label, does that have any implications for the size of the repository? Specifically, I'm thinking about pull requests that are never merged. Will these new refs cause us to accumulate objects that would otherwise be garbage collected? Could that significantly contribute to the size of the repository? |
Yes and no. If you fetch these refs they will point at objects you might not otherwise fetch, and that can make your local clone larger. But if, for example, you already configured git to pull all the PR heads, then this will have a neglible impact. On the GitHub side all the commits that we would add a ref to area already kept alive by the PR head refs, so the only difference is the extra storage for the refs themselves. By not making the refs tags, you don't fetch them by default so for local users this is lower impact than the original proposal since changes require an explicit opt-in. |
@jgraham this patch now configures GitHub Actions to manage two separate git refs: |
@jgraham would you mind taking another look at this? |
1 similar comment
@jgraham would you mind taking another look at this? |
Thanks, @jgraham! I'll do the honors |
…s#17680) * Introduce automation to support pull rqst previews Today, http://w3c-test.org automatically fetches and publishes the code from "preview-enabled" pull requests to WPT. It defines eligible pull requests as those authored by project collaborators and those where a collaborator has left a special GitHub comment. That mechanism has a number of drawbacks: - The configuration is hidden. Only those with admin rights can review the GitHub webhook upon which the solution relies. - Pull request state is opaque. It's not possible to know when/if a pull request has been recognized by w3c-test.org from the pull request or from WPT (though this information is available at the undocumented page http://w3c-test.org/submissions/). - Server state (i.e. the collection of which pull requests are considered "active") is likewise opaque. Also, in the absence of a backup mechanism, it is non-recoverable following a system crash - Previews cannot be disabled This patch is one component of an attempt to reimplement this service in a way that gives more control to the WPT administration and stores more state in the WPT repository. - Maintains state via git tags. This avoids the complexities of an external storage system (access credentials, back up policies, etc.) - Observes eligibility using GitHub pull request labels. This makes the state of the system apparent from the pull requests themselves. It also provides a more ergonomic and discoverable mechanism for enabling/disabling previews This is facilitated by a script that is executed as a GitHub Action. Although the script relies on a secret access token, it is *not* limited to use for pull requests from trusted collaborators due to the way GitHub Actions are executed [1] > ### Pull request events for forked repositories > > [...] > > #### Pull request with base and head branches in different repositories > > The base repository receives a `pull_request event` where the SHA is > the latest commit of base branch and ref is the base branch. With this in place, a far simpler "preview" server can be built. The simplified version will need no privileged information; it will only need to poll the git repository for tags and synchronize its deployment according to the result. [1] https://developer.github.com/actions/managing-workflows/workflow-configuration-options/#pull-request-events-for-forked-repositories * fixup! Introduce automation to support pull rqst previews Add support for Python 3 * fixup! Introduce automation to support pull rqst previews * fixup! Introduce automation to support pull rqst previews Use generic git refs instead of tags * fixup! Introduce automation to support pull rqst previews * fixup! Introduce automation to support pull rqst previews React to events from closed pull requests * fixup! Introduce automation to support pull rqst previews
Today, http://w3c-test.org automatically fetches and publishes the code
from "preview-enabled" pull requests to WPT. It defines eligible pull
requests as those authored by project collaborators and those where a
collaborator has left a special GitHub comment. That mechanism has a
number of drawbacks:
the GitHub webhook upon which the solution relies.
request has been recognized by w3c-test.org from the pull request or
from WPT (though this information is available at the undocumented
page http://w3c-test.org/submissions/).
considered "active") is likewise opaque. Also, in the absence of a
backup mechanism, it is non-recoverable following a system crash
This patch is one component of an attempt to reimplement this service in
a way that gives more control to the WPT administration and stores more
state in the WPT repository.
external storage system (access credentials, back up policies, etc.)
state of the system apparent from the pull requests themselves. It
also provides a more ergonomic and discoverable mechanism for
enabling/disabling previews
This is facilitated by a script that is executed as a GitHub Action.
Although the script relies on a secret access token, it is not limited
to use for pull requests from trusted collaborators due to the way
GitHub Actions are executed [1]
With this in place, a far simpler "preview" server can be built. The
simplified version will need no privileged information; it will only
need to poll the git repository for tags and synchronize its deployment
according to the result.
[1] https://developer.github.com/actions/managing-workflows/workflow-configuration-options/#pull-request-events-for-forked-repositories
I used the wpt-actions-test repository to build this. Here are two pull requests which demonstrate the functionality:
I have not yet written the corresponding server script. Although it should be fairly simple, I wanted to get feedback on this approach before going further.