graydon / bors

Integration robot for buildbot and github
427 stars 49 forks source link

Alternative rollup implementation for the rust branch #47

Closed barosl closed 9 years ago

barosl commented 9 years ago

This is an alternative implementation of #46.

brson commented 9 years ago

This looks good to me though the logic around self.merge_sha was a bit hard for me to follow, and my python is weak.

Unfortunately I can't test it right now. I may be able to tonight, but if not I will tomorrow.

ISTM that if the rollup fails the failure will be posted to whichever PR triggered the test, but then bors will proceed to the other rollup PRs and do the test again (which will fail again, until all the PRs have triggered a failed rollup). This would be counter-productive.

I'm wondering what the workflow is for rollups that fail. Currently rollups have a PR and if there's a failure, the rollup author fixes it manually and tries again, or removes. Under this scheme I'd imagine someone would figure out which PR caused the failure, remove the 'rollup' comment or 'r-' it, then '@bors: retry' the rollup. Does that sound right?

brson commented 9 years ago

With this arrangement that doesn't create a new PR it won't be possibly to use the hypothetical '@bors: try' to test a rollup before attempting to land. I'm not sure this is a problem, but my hunch is that rollups often fail a number of times since they're so big.

If it becomes desirable to try rollups it might be nice to support tagging rollups independently of r+, like one comment with '@bors: rollup' (though maybe your regexp supports just the word 'rollup'?).

barosl commented 9 years ago

@brson

This looks good to me though the logic around self.merge_sha was a bit hard for me to follow, and my python is weak.

I also think the code around self.merge_sha is somewhat ugly and hacky. In fact, the logic was copied from the master branch and does not fit well with the one-by-one strategy of the rust branch. However I cannot think of a better solution to this...

if the rollup fails the failure will be posted to whichever PR triggered the test, but then bors will proceed to the other rollup PRs and do the test again

This is true, but bors won't proceed to one of the remaining rollup PRs immediately, because they all have lower priority than the normal PRs. (We triggered the rollup by setting p=99.) So while bors trying the normal PRs, we can have time to review the rollup and rearrange them.

remove the 'rollup' comment or 'r-' it, then '@bors: retry' the rollup. Does that sound right?

Yes, that would be a typical approach to a failed rollup.

it might be nice to support tagging rollups independently of r+, like one comment with '@bors: rollup' (though maybe your regexp supports just the word 'rollup'?).

Right, rollup is already independent from r+ by the definition of the regular expression, so it can be freely added or removed regardless of the existence of r+. I think this is reasonable.

brson commented 9 years ago

I've deployed this to production. @barosl will you write up a summary of how to do rollups to discuss.rust-lang.org?

Gankra commented 9 years ago

:confetti_ball:

barosl commented 9 years ago

@brson Done. Batched merge (“rollup”) feature has landed on bors