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

Introduce automation to support pull rqst previews #17680

Merged
merged 7 commits into from Aug 1, 2019

Conversation

jugglinmike
Copy link
Contributor

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


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.

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

@sideshowbarker sideshowbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor copy edit

docs/writing-tests/submission-process.md Outdated Show resolved Hide resolved
Copy link
Contributor

@sideshowbarker sideshowbarker left a 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

@jgraham
Copy link
Contributor

jgraham commented Jul 10, 2019

So I think I like the general idea here. Some thoughts

  • This seems like 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.

  • Using tags as the refs has the unfortunate side effect that we fetch these by default. I think I would prefer to use a different namespace under refs/ e.g. refs/open_pulls for all open PRs and refs/previews for things labelled as preview.

  • The amount of test code is >> than the amount of functional code. Although I admire the commitment to testing, this is creating a different kind of maintainance burden, and tests involving running servers can be unreliable. I wonder if we have considered using something like requests-mock to remove some first-party code here.

@jugglinmike
Copy link
Contributor Author

  • This seems like 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.

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.

  • Using tags as the refs has the unfortunate side effect that we fetch these by default. I think I would prefer to use a different namespace under refs/ e.g. refs/open_pulls for all open PRs and refs/previews for things labelled as preview.

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.

  • The amount of test code is >> than the amount of functional code. Although I admire the commitment to testing, this is creating a different kind of maintainance burden, and tests involving running servers can be unreliable. I wonder if we have considered using something like requests-mock to remove some first-party code here.

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.

@jgraham
Copy link
Contributor

jgraham commented Jul 12, 2019

  • This seems like 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.

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?

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.

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.

This is true.

  • Using tags as the refs has the unfortunate side effect that we fetch these by default. I think I would prefer to use a different namespace under refs/ e.g. refs/open_pulls for all open PRs and refs/previews for things labelled as preview.

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.

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.

@jugglinmike
Copy link
Contributor Author

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?

@jgraham
Copy link
Contributor

jgraham commented Jul 12, 2019

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.

@jugglinmike jugglinmike reopened this Jul 15, 2019
@jugglinmike
Copy link
Contributor Author

@jgraham this patch now configures GitHub Actions to manage two separate git refs: refs/prs-open/gh-XXX and refs/prs-labeled-for-preview/gh-XXX

@jugglinmike
Copy link
Contributor Author

@jgraham would you mind taking another look at this?

1 similar comment
@jugglinmike
Copy link
Contributor Author

@jgraham would you mind taking another look at this?

@jugglinmike
Copy link
Contributor Author

Thanks, @jgraham! I'll do the honors

@jugglinmike jugglinmike merged commit 89f8327 into web-platform-tests:master Aug 1, 2019
natechapin pushed a commit to natechapin/wpt that referenced this pull request Aug 23, 2019
…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
@zcorpan zcorpan removed the .github label Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants