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

Recognise 'Branch cannot be merged' error and retry later #292

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nirizr
Copy link

@nirizr nirizr commented Feb 24, 2021

I've seen the 'Branch cannot be merged' several times due to a race condition between margebot and pipelines been rerun.

If this occurs, IMHO margebot shouldn't necessarily give up immediately but instead retry again which is achieved by keeping marge bot the assignee.

WDYT?

@nirizr nirizr force-pushed the nirizr/dont_unassign_cannot_merge branch from 4bf0f35 to 903dd5e Compare February 24, 2021 09:41
@snim2
Copy link
Contributor

snim2 commented Feb 24, 2021

Maybe! Another option might be to call this function which implements a simple linear backoff: https://github.com/smarkets/marge-bot/pull/265/files -- BTW do you know why this error occurs?

@nirizr
Copy link
Author

nirizr commented Feb 24, 2021

Thanks for your reply!

I was actually working on an outdated version (just recently rebased on latest version) when I originally made this commit internally a while ago. The commit you pointed out might be making this PR redundant.

When I investigated it in the past, it was happening because while marge was trying to merge, another MR was merged in manually by a user, which required the MR marge was merging to be rebased / updated (per our gitlab configuration) before it could be merged.

If you expect your change to fix this issue I can report back if I ever get this issue again (now with v0.9.5).
Please lmk and I'll revert this change internally and close this PR.

@snim2
Copy link
Contributor

snim2 commented Mar 10, 2021

That's interesting @nirizr - I'm not sure whether my PR would fix this or not. My changes assume that the MR is good to merge, but that the GitLab API endpoint isn't yet returning the correct result. In your situation, you need Marge to realise that the MR now cannot be merged, and then rebase again.

I'd be interested to find out whether you still experience this bug with version 0.9.5, but my suspicion is that you will.

@nirizr
Copy link
Author

nirizr commented Mar 11, 2021

If you don't believe your fix applies here. What could be the risk of merging it in? Would you prefer waiting until I can confirm its still helpful?

I haven't seen that behavior with v0.9.5 but we're more frequently using Marge now (compared to manual merging we used to do a lot when I just brought Marge in).

@snim2
Copy link
Contributor

snim2 commented Mar 11, 2021

Hi @nirizr I'm not a maintainer of this project, so I can't help with merging your change. I think it would be useful to confirm that the bug still occurs in v0.9.5, but that's just my opinion!

@nirizr
Copy link
Author

nirizr commented Mar 11, 2021

Yea, I was asking for your opinion :)

@snim2
Copy link
Contributor

snim2 commented Mar 11, 2021

Ha! Well, personally I'm always cautious about merging, but maybe that's just me ;)

@qqshfox
Copy link
Contributor

qqshfox commented Apr 30, 2021

Hi, wondering if this is still needed for v0.9.5+?

@nirizr
Copy link
Author

nirizr commented Apr 30, 2021

Hi @qqshfox, thanks for taking the time!

I believe I've encountered the error still with latest marge version.

Keep in mind, though, that company I work for was recently bought so we no longer use Marge bot.

@qqshfox
Copy link
Contributor

qqshfox commented Apr 30, 2021

Interesting. We've been using v0.9.5 for 2+ months at Smarkets, and not seen this again.

I'll leave this open in case anyone encounters this issue with the latest version again.

@nirizr
Copy link
Author

nirizr commented Apr 30, 2021

We encountered it because users were also merging (and pushing to master, btw) manually. Do you do that at Smarkets?
Either way waiting for confirmation sounds good.

LMK if there's anything else I can maybe help with.

@qqshfox
Copy link
Contributor

qqshfox commented Apr 30, 2021

No. We don't allow pushing to master.

But wait, is that the expected behavior to give up merging when there are new commits in master?

@nirizr
Copy link
Author

nirizr commented Apr 30, 2021

Our gitlab configuration was such that only fast forward merges were allowed (so users would often rebase on top of master just before merging, to run our pipeline on the most up to date code) into master.

From memory, what would happen was this:

  1. A merge was permitted (because pipelines passed and merge was a FF merge).
  2. Marge bot would pull all MRs in the project and cycle through until it reaches MR.
  3. User would than manually merge (i.e click the "merge MR" button on gitlab) or push some changes to master (because it was allowed in certain cases).
  4. Marge bot would start the merging sequence / flow.
  5. Merge bot would issue the merge MR API request to gitlab.
  6. Gitlab would fail because MR is no longer permitted (it now requires a rebase to be FFed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants