Add rollup logic #458
Add rollup logic #458
Conversation
I've used it very often. It's super helpful for creating a new rollup based on a failed one where you've kicked out a single PR. I'm pretty sure everyone who does rollups uses it all the time. You don't want to manually reassemble the failed rollup -1 bad PR (+N new ones) when there are like 30+ PRs in the queue. Now, I've heard that this bors impl will have other tricks up its sleeve eventually esp. for creating multiple rollups at once / staggered, so maybe such a link will indeed be made obsolete via some other functionality / mechanism. |
I think that button is indeed quite useful. However, we don't need to implement it in this PR, it can be follow-up work. |
Kobzol
left a comment
Kobzol
left a comment
There was a problem hiding this comment.
This looks awesome, thank you so much! I went through it and left a bunch of comments.
Apart from the comments, it would be great to have tests for this (doesn't need to be in this PR, let's do it in a follow-up). I'm imagining something like this:
- The test context will gain a new method, something like
.rollup(<list-of-prs>)(we can just hardcode the default repo name inside of it). - The method will post a request to the
/oauth/callbackendpoint - We will have to mock any GitHub requests done by the rollup handler.
- We wait for the rollup to be done (should be enough to wait for the OAuth callback HTTP request finishing) and then assert that the rollup PR was created.
src/github/server.rs
Outdated
|
|
||
| // Merge each PR's commits into the rollup branch | ||
| for pr in rollup_prs { | ||
| let pr_github = gh_client |
There was a problem hiding this comment.
Loading the PRs can take some times, as it's up to e.g. 20 roundtrips to GitHub. As a follow-up, it would be great to do some mini-benchmark to see how long it takes to load e.g. 20 PRs sequentially, vs creating e.g. FuturesUnordered or something that can buffer futures and load e.g. 5 PRs at a time, to make this faster, as it can be trivially parallelized.
|
Thank you! The PR needs a rebase, and maybe a squash (17 commits is a bit too much), but other than that I think I'm ok with merging! We can take a look at parallelizing the PR fetch later. Awesome job! ❤️ |
Sakib25800
force-pushed
the
rollup-logic
branch
2 times, most recently
from
9aaa11d to
eb407b8
Compare
Sakib25800
force-pushed
the
rollup-logic
branch
from
eb407b8 to
7532875
Compare
3 participants
This PR adds the rollup logic. You can select certain PRs and then create a rollup.
I did not add the "create a similar PR" text in the rollup PR description as I'm not sure it's used - I can add that in another PR if needed.
The rollup logic works very similarly to homu's.