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

internal/contributebot: merge in pull requests triggered by comment #687

Closed
zombiezen opened this issue Nov 13, 2018 · 4 comments
Closed
Labels
enhancement New feature or request
Milestone

Comments

@zombiezen
Copy link
Contributor

Is your feature request related to a problem? Please describe.

From a comment on #630:

[Go Cloud contributors sit in front of their PRs], since the process for submission is:

while branch is not up-to-date with master {
  press update branch button
  wait for Travis run to finish
}
click squash-and-merge

This loop is especially expensive, as the longer you wait, the more likely the branch is not up-to-date and thus causing the cycle to increase iterations.

Describe the solution you'd like

Contribute Bot could be in charge of merging in changes via a comment like /merge, only reporting back if the build failed. The commit message could be derived from the PR title and first comment.

Describe alternatives you've considered

We could use off-the-shelf solutions, but the amount of effort required to get them working would be roughly equivalent to just getting Contribute Bot to do it.

Solutions we found:

@zombiezen zombiezen added the enhancement New feature or request label Nov 13, 2018
@zombiezen zombiezen added this to the Unplanned milestone Nov 13, 2018
@zombiezen
Copy link
Contributor Author

/cc @ijt FYI

@ijt ijt self-assigned this Nov 16, 2018
@ijt ijt assigned clausti and unassigned ijt Nov 26, 2018
@zombiezen zombiezen assigned zombiezen and unassigned clausti Dec 17, 2018
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 17, 2018
These events will be necessary for merges, but currently aren't used.
Adding them now to clear them from the subscription backlog.

Updates google#687
zombiezen added a commit that referenced this issue Dec 17, 2018
These events will be necessary for merges, but currently aren't used.
Adding them now to clear them from the subscription backlog.

Updates #687
@zombiezen zombiezen added the in progress This is being actively worked on label Dec 18, 2018
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 18, 2018
Detailed design is still a little far away, but I want to ensure that
the high-level functionality presented to the maintainers is desirable
before proceeding.

Updates google#687
zombiezen added a commit that referenced this issue Dec 21, 2018
#1009)

Detailed design is still a little far away, but I want to ensure that
the high-level functionality presented to the maintainers is desirable
before proceeding.

Updates #687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
…tions

Part of the merge functionality. Already added to production Contribute
Bot application configuration. No-ops added in previous commits.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
Makes RoundTrip easier to understand, but also allows the cached token
to be reused for other purposes (like Git tool access).

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
WIP; DO NOT SUBMIT missing tests

This is not hooked up to anything else in Contribute Bot yet. Subsequent
commits will call syncPullRequest when they need to merge a pull
request's branch into the pull request branch.

I use a Git subprocess in gitSync for two reasons:

1.  The merge process can be tested hermetically.
2.  Using Git directly does not consume GitHub API quota, whereas using
    the [GitHub Git Database API][] would.

[GitHub Git Database API]: https://developer.github.com/v3/git/

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
Contribute Bot will use this to track pull requests that should be kept
in sync with the head branch. Adding the label before Contribute Bot
uses it.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
…upstream

WIP; DO NOT SUBMIT

This is an incremental (but significant) step toward accomplishing google#687.
It does not submit PRs, but it manages them so that they are more likely
to be in a mergeable state. Since this is taking some shortcuts, I am
not updating the design doc with details of this change yet, but this
proves (to me at least) the feasibility of an approach where all state
is stored in GitHub. Hopefully this eases enough maintainer toil that I
can do design work for the remaining elements on a more relaxed
schedule.

When a pull request is labeled with "ready to submit", Contribute Bot
calls syncPullRequest to update the pull request. Using a label makes
validation easy because only users with write access to a repository
can make this change.

On any branch push to the repository, Contribute Bot will search for any
open pull requests with the "ready to submit" label that have a base
branch of the updated branch. Contribute Bot will call syncPullRequest
for all the matching pull requests.

If any errors occur while merging, Contribute Bot will remove the "ready
to submit" label and leave a comment. This gives notification of
required manual intervention.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
…tions

Part of the merge functionality. Already added to production Contribute
Bot application configuration. No-ops added in previous commits.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
Makes RoundTrip easier to understand, but also allows the cached token
to be reused for other purposes (like Git tool access).

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
This is not hooked up to anything else in Contribute Bot yet. Subsequent
commits will call syncPullRequest when they need to merge a pull
request's branch into the pull request branch.

I use a Git subprocess in gitSync for two reasons:

1.  The merge process can be tested hermetically.
2.  Using Git directly does not consume GitHub API quota, whereas using
    the [GitHub Git Database API][] would.

[GitHub Git Database API]: https://developer.github.com/v3/git/

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
Contribute Bot will use this to track pull requests that should be kept
in sync with the head branch. Adding the label before Contribute Bot
uses it.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
…upstream

This is an incremental (but significant) step toward accomplishing google#687.
It does not submit PRs, but it manages them so that they are more likely
to be in a mergeable state. Since this is taking some shortcuts, I am
not updating the design doc with details of this change yet, but this
proves (to me at least) the feasibility of an approach where all state
is stored in GitHub. Hopefully this eases enough maintainer toil that I
can do design work for the remaining elements on a more relaxed
schedule.

When a pull request is labeled with "ready to submit", Contribute Bot
calls syncPullRequest to update the pull request. Using a label makes
validation easy because only users with write access to a repository
can make this change.

On any branch push to the repository, Contribute Bot will search for any
open pull requests with the "ready to submit" label that have a base
branch of the updated branch. Contribute Bot will call syncPullRequest
for all the matching pull requests.

If any errors occur while merging, Contribute Bot will remove the "ready
to submit" label and leave a comment. This gives notification of
required manual intervention.

I've changed the built container to include a copy of Git. This does
bump up the container to requiring debian-slim, but I don't see a good
way around that.

Updates google#687
@zombiezen
Copy link
Contributor Author

@vangent and I brainstormed how to move this feature forward incrementally before the holidays. Basically, this can be broken down into two major technical pieces:

  1. Keep requested PRs up-to-date with master — call this auto-sync. This requires watching for push events and label events and performing Git merges where appropriate.
  2. Squash and merge PRs into master when all checks pass — call this auto-merge. This requires watching for check run events, verifying that no unapproved commits slip in between the time a maintainer kicks off the submit and the merge event, and then invoking the GitHub API to merge the pull request.

Auto-merge is not very useful without auto-sync, but auto-sync is useful even without auto-merge.

I have a working auto-sync implementation ready to go. To keep the implementation simple for the first pass, the trigger is a GitHub label named "ready to submit" instead of a comment. You can see this in action on zombiezen/go-cloud-bot-test#8, one of my test PRs: I added the "ready to submit" label, I made an upstream commit, the bot merged it into the PR, and then I merged the change in manually.

My WIP branch is here: master...zombiezen:contribute-bot-merge

I'll start sending out PRs: one per commit on the branch. The first handful are small cleanups, but the last few add the real functionality. Once this is in, I'll update the detailed design with a fuller understanding of both the auto-sync and auto-merge.

zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
…tions

Part of the merge functionality. Already added to production Contribute
Bot application configuration. No-ops added in previous commits.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
Makes RoundTrip easier to understand, but also allows the cached token
to be reused for other purposes (like Git tool access).

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
This is not hooked up to anything else in Contribute Bot yet. Subsequent
commits will call syncPullRequest when they need to merge a pull
request's branch into the pull request branch.

I use a Git subprocess in gitSync for two reasons:

1.  The merge process can be tested hermetically.
2.  Using Git directly does not consume GitHub API quota, whereas using
    the [GitHub Git Database API][] would.

[GitHub Git Database API]: https://developer.github.com/v3/git/

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
Contribute Bot will use this to track pull requests that should be kept
in sync with the head branch. Adding the label before Contribute Bot
uses it.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 27, 2018
…upstream

This is an incremental (but significant) step toward accomplishing google#687.
It does not submit PRs, but it manages them so that they are more likely
to be in a mergeable state. Since this is taking some shortcuts, I am
not updating the design doc with details of this change yet, but this
proves (to me at least) the feasibility of an approach where all state
is stored in GitHub. Hopefully this eases enough maintainer toil that I
can do design work for the remaining elements on a more relaxed
schedule.

When a pull request is labeled with "ready to submit", Contribute Bot
calls syncPullRequest to update the pull request. Using a label makes
validation easy because only users with write access to a repository
can make this change.

On any branch push to the repository, Contribute Bot will search for any
open pull requests with the "ready to submit" label that have a base
branch of the updated branch. Contribute Bot will call syncPullRequest
for all the matching pull requests.

If any errors occur while merging, Contribute Bot will remove the "ready
to submit" label and leave a comment. This gives notification of
required manual intervention.

I've changed the built container to include a copy of Git. This does
bump up the container to requiring debian-slim, but I don't see a good
way around that.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 28, 2018
…tions

Part of the merge functionality. Already added to production Contribute
Bot application configuration. No-ops added in previous commits.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 28, 2018
Makes RoundTrip easier to understand, but also allows the cached token
to be reused for other purposes (like Git tool access).

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 28, 2018
This is not hooked up to anything else in Contribute Bot yet. Subsequent
commits will call syncPullRequest when they need to merge a pull
request's branch into the pull request branch.

I use a Git subprocess in gitSync for two reasons:

1.  The merge process can be tested hermetically.
2.  Using Git directly does not consume GitHub API quota, whereas using
    the [GitHub Git Database API][] would.

[GitHub Git Database API]: https://developer.github.com/v3/git/

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 28, 2018
Contribute Bot will use this to track pull requests that should be kept
in sync with the head branch. Adding the label before Contribute Bot
uses it.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 28, 2018
…upstream

This is an incremental (but significant) step toward accomplishing google#687.
It does not submit PRs, but it manages them so that they are more likely
to be in a mergeable state. Since this is taking some shortcuts, I am
not updating the design doc with details of this change yet, but this
proves (to me at least) the feasibility of an approach where all state
is stored in GitHub. Hopefully this eases enough maintainer toil that I
can do design work for the remaining elements on a more relaxed
schedule.

When a pull request is labeled with "ready to submit", Contribute Bot
calls syncPullRequest to update the pull request. Using a label makes
validation easy because only users with write access to a repository
can make this change.

On any branch push to the repository, Contribute Bot will search for any
open pull requests with the "ready to submit" label that have a base
branch of the updated branch. Contribute Bot will call syncPullRequest
for all the matching pull requests.

If any errors occur while merging, Contribute Bot will remove the "ready
to submit" label and leave a comment. This gives notification of
required manual intervention.

I've changed the built container to include a copy of Git. This does
bump up the container to requiring debian-slim, but I don't see a good
way around that.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 28, 2018
…tions

Part of the merge functionality. Already added to production Contribute
Bot application configuration. No-ops added in previous commits.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 28, 2018
Makes RoundTrip easier to understand, but also allows the cached token
to be reused for other purposes (like Git tool access).

Updates google#687
zombiezen added a commit that referenced this issue Dec 28, 2018
…tions (#1045)

Part of the merge functionality. Already added to production Contribute
Bot application configuration. No-ops added in previous commits.

Updates #687
zombiezen added a commit that referenced this issue Dec 28, 2018
…od (#1046)

Makes RoundTrip easier to understand, but also allows the cached token
to be reused for other purposes (like Git tool access).

Updates #687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 28, 2018
This is not hooked up to anything else in Contribute Bot yet. Subsequent
commits will call syncPullRequest when they need to merge a pull
request's branch into the pull request branch.

I use a Git subprocess in gitSync for two reasons:

1.  The merge process can be tested hermetically.
2.  Using Git directly does not consume GitHub API quota, whereas using
    the [GitHub Git Database API][] would.

[GitHub Git Database API]: https://developer.github.com/v3/git/

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 28, 2018
Contribute Bot will use this to track pull requests that should be kept
in sync with the head branch. Adding the label before Contribute Bot
uses it.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Dec 28, 2018
…upstream

This is an incremental (but significant) step toward accomplishing google#687.
It does not submit PRs, but it manages them so that they are more likely
to be in a mergeable state. Since this is taking some shortcuts, I am
not updating the design doc with details of this change yet, but this
proves (to me at least) the feasibility of an approach where all state
is stored in GitHub. Hopefully this eases enough maintainer toil that I
can do design work for the remaining elements on a more relaxed
schedule.

When a pull request is labeled with "ready to submit", Contribute Bot
calls syncPullRequest to update the pull request. Using a label makes
validation easy because only users with write access to a repository
can make this change.

On any branch push to the repository, Contribute Bot will search for any
open pull requests with the "ready to submit" label that have a base
branch of the updated branch. Contribute Bot will call syncPullRequest
for all the matching pull requests.

If any errors occur while merging, Contribute Bot will remove the "ready
to submit" label and leave a comment. This gives notification of
required manual intervention.

I've changed the built container to include a copy of Git. This does
bump up the container to requiring debian-slim, but I don't see a good
way around that.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Jan 7, 2019
Contribute Bot will use this to track pull requests that should be kept
in sync with the head branch. Adding the label before Contribute Bot
uses it.

Updates google#687
zombiezen added a commit to zombiezen/go-cloud that referenced this issue Jan 7, 2019
…upstream

This is an incremental (but significant) step toward accomplishing google#687.
It does not submit PRs, but it manages them so that they are more likely
to be in a mergeable state. Since this is taking some shortcuts, I am
not updating the design doc with details of this change yet, but this
proves (to me at least) the feasibility of an approach where all state
is stored in GitHub. Hopefully this eases enough maintainer toil that I
can do design work for the remaining elements on a more relaxed
schedule.

When a pull request is labeled with "ready to submit", Contribute Bot
calls syncPullRequest to update the pull request. Using a label makes
validation easy because only users with write access to a repository
can make this change.

On any branch push to the repository, Contribute Bot will search for any
open pull requests with the "ready to submit" label that have a base
branch of the updated branch. Contribute Bot will call syncPullRequest
for all the matching pull requests.

If any errors occur while merging, Contribute Bot will remove the "ready
to submit" label and leave a comment. This gives notification of
required manual intervention.

I've changed the built container to include a copy of Git. This does
bump up the container to requiring debian-slim, but I don't see a good
way around that.

Updates google#687
@zombiezen
Copy link
Contributor Author

Progress update (copied from a comment on #1073):

I was assuming that the "Maintainers Can Modify" would include GitHub applications installed on a repository, but a sample PR (zombiezen/go-cloud-bot-test#9) shows that this is not the case. 🙁

Good news: I have a workaround for the auth issue. GitHub Applications can obtain user-to-server credentials so that the server can perform actions on behalf of a user instead of the application itself. I have manually tested that access tokens obtained in this way can be used to authorize pushes to branches in pull request forks that have the "Maintainers Can Modify" bit.

Bad news: things get more complicated. Instead of just keeping credentials for the application, Contribute Bot now needs to store credentials per maintainer and serve a 3-legged OAuth flow. When performing syncs, Contribute Bot needs to query for who requested the sync and authorize using their credentials.

zombiezen added a commit to zombiezen/go-cloud that referenced this issue Jan 18, 2019
In preparation for google#687 (which requires the frontend to persist auth
information), I wanted to convert the webhook to use Go Cloud to
support a more advanced configuration. This uses the second generation
App Engine runtime.
@zombiezen
Copy link
Contributor Author

Unassigning for now. There's higher priority work to do, and #1181 alleviated most of the toil associated with this. The next steps are clear, but just require work because Contribute Bot would need to store state.

@zombiezen zombiezen removed the in progress This is being actively worked on label Feb 13, 2019
@zombiezen zombiezen removed their assignment Feb 13, 2019
@eliben eliben closed this as completed Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants