let miq_bot merge - Githubissues

durandom commented 8 years ago

Sometimes PRs get merged and they break master. I guess this is due to a race condition, like when travis ran, everything was good but meanwhile another PR got merged which would let the former PR fail. The person about to merge the first PR still sees the old green travis and happily merges.

Let miq_bot do the work.

label a PR with [merge] or address miq-bot with a merge command (permissions apply)
miq-bot queues PR up for merging, so no merging in parallel
if travis is green: merge via github api

the downside: We can only be as fast as our test suite. Now it runs ~30mins, so we would get 2 merges / hour.

Some think, having a straight git history is nice. Although thats opinonated, I guess: https://www.atlassian.com/git/articles/git-team-workflows-merge-or-rebase/ In case we want that:

pull PR into miq-bots repo hub checkout https://github.com/github/hub/pull/134
rebase PR ff-only
run travis against miq-bots PR
in case travis is green: merge miq-bots PR

I guess the former part would be pretty easy to build and would mainly rely on discipline not to hit the big green button :black_square_button: I'd be happy to implement that if we want to do this.

In the future we could even run a larger test suite on the merge queue (eg. some of QEs automated tests).

@Fryguy what do you think? And please pull in those to whom it may concern :)

matthewd commented 8 years ago

IMO actual merge conflicts just don't happen often enough to worry about.

But if we really want this, we should just use @homu.

jrafanie commented 8 years ago

This would save me some annoyances as the reviewer and time as the pr author. Combined with irc/gitter messages when the build fails via the travis.yml, I think we can do this with no code on our part. homu + Travis notifications would enable this.

jrafanie commented 8 years ago

Or if @fryguy would wait to ask me to review something until it's green.:trollface:

durandom commented 8 years ago

IMO actual merge conflicts just don't happen often enough to worry about.

maybe I just get it wrong, but arent these all merges to the master while master was broken? And probably the first in each timeframe was the bad one? gem install travis ; travis history -d --branch master --all | grep failed

And great that @homu exists, I thought there is something already out there, just could not find it :+1: Should we use their hosted, free service or run the thing on our own?

jrafanie commented 8 years ago

@durandom I see what you're saying... that's not merge conflicts but merges while master is red. Yes, it would be nice if a bot would not auto-merge when master is red. That would require someone to manually merge the fix to make it green it again before the bot continues merging as usual.

I only know of @homu and don't know what it does or does not do. I like the idea of not writing our own version of it.

matthewd commented 8 years ago

@durandom without actually checking the failures, I suspect the majority, at least, are failures due to changes in dependencies.

Strictly speaking, insisting on running the tests immediately before the merge would prevent that, but what do we gain? Master is technically green, because the most recent build ran at a time it was able to succeed... but if you re-ran the tests now, they'd fail.

Now, something that would take an "LGTM" comment, and (if no new changes have been submitted since,) merge when the status goes green, does seem valuable -- that addresses @jrafanie's "I have looked at these changes, and they seem okay, but I now need to wait for green before I can merge".

Rearranging the actual test-running process just seems like a lot of overkill for an incredibly rare problem, to me. :confused:

durandom commented 8 years ago

yeah, I thought @matthewd meant merges while master is red/broken, because you cant merge a PR if it has conflicts. So this is all about keeping master stable. And by having a gatekeeper like @homu it cant break, because the bot would only merge on green.

durandom commented 8 years ago

Rearranging the actual test-running process

I think the test running and travis setup would remain the same. Just the merging is done by a bot. And I dont know how rare this problem really is. Since I am on board, I remember a couple of times people on gitter talking about a broken master. And I guess this affects some people working on their local branches too.

But after all, you guys have to weigh in how this really is a problem. It popped to my mind and I used this issue to write it down :)

matthewd commented 8 years ago

I meant what I said. :unamused: You can't merge a PR if it has detected conflicts, due to physically overlapping changes. The theory behind a "re-run the tests with the PR + current master immediately before merging" strategy is: since the PR's previous test run, master may have gained conflicting (but not overlapping) changes, such that the sum of the changesets is red even though they are individually green.

But yeah: "it cant break, because the bot would only merge on green" seems to be misunderstanding what actually causes master to go red.

durandom commented 8 years ago

@matthewd so the master does not turn red, because some code in the PR breaks it, but changes in dependent external libraries? Just trying to understand this

matthewd commented 8 years ago

Confirm. See the first green builds after the red ones in your report... I doubt you'll find many that are untangling a conflict between recent merges: I believe they'll either be fixing an external problem (either by changing our code to match, or pinning a version -- but unrelated to the preceding code changes), or dealing with a PR that was accidentally merged when it was already failing on its own.

chrisarcand commented 8 years ago

@durandom A more concrete example of "changes in dependent external libraries"...

Billy updates the test suite to RSpec 3 and disables a certain feature found in RSpec 2. Tests pass, PR's ready.
Sue writes a brand new feature. She writes tests using the certain feature in RSpec 2 that Billy is removing at the same time. Billy's changes aren't in master yet. So at latest master, Sue's test happily pass as well.
Billy's PR is merged! :tada:
Sue's PR is still green, because it was tested with RSpec 2. Plus, it doesn't literally conflict with anything in Billy's PR, so is totally mergeable. Green means go! Sue's PR is merged.

Result: In master, test suite is now broken from Sue's merge even though Sue's PR was happily green. Dependent external libraries.

This example is silly because it's just tests, I use it because I recall it happening to us this past fall :) But you can extend that to any functionality in any framework used in actual application code that gets changed/removed in one PR and added in another.

matthewd commented 8 years ago

@chrisarcand no, that's the sort of thing @durandom's proposal would prevent, that I'm arguing ~never actually happens

chrisarcand commented 8 years ago

:neutral_face:

Now I'm confused and will have to reread. Sorry :confused:

durandom commented 8 years ago

@chrisarcand I'm glad, I'm not the only one that didnt spot the difference right away. If we would add Gemfile.lock to git, then changes in external libraries would not strike us. But we dont, so a change in e.g. rails master could break our master without our code being changed. This is not covered by merging in a queue... I did not think of this when opening this issue, but thought its only those 'old green merges'

chrisarcand commented 8 years ago

Oooh I understand now. :flushed: Thanks @durandom

miq-bot commented 4 years ago

This issue has been automatically marked as stale because it has not been updated for at least 3 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the traige process documentation.

ManageIQ / miq_bot

let miq_bot merge #165