Skip to content

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

Open
nirizr wants to merge 1 commit into
smarkets:masterfrom
nirizr:nirizr/dont_unassign_cannot_merge
Open

Recognise 'Branch cannot be merged' error and retry later#292
nirizr wants to merge 1 commit into
smarkets:masterfrom
nirizr:nirizr/dont_unassign_cannot_merge

Conversation

@nirizr

@nirizr nirizr commented Feb 24, 2021

Copy link
Copy Markdown

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

snim2 commented Feb 24, 2021

Copy link
Copy Markdown
Contributor

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

nirizr commented Feb 24, 2021

Copy link
Copy Markdown
Author

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

snim2 commented Mar 10, 2021

Copy link
Copy Markdown
Contributor

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

nirizr commented Mar 11, 2021

Copy link
Copy Markdown
Author

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

snim2 commented Mar 11, 2021

Copy link
Copy Markdown
Contributor

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

nirizr commented Mar 11, 2021

Copy link
Copy Markdown
Author

Yea, I was asking for your opinion :)

@snim2

snim2 commented Mar 11, 2021

Copy link
Copy Markdown
Contributor

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

@qqshfox

qqshfox commented Apr 30, 2021

Copy link
Copy Markdown
Contributor

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

@nirizr

nirizr commented Apr 30, 2021

Copy link
Copy Markdown
Author

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

qqshfox commented Apr 30, 2021

Copy link
Copy Markdown
Contributor

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

nirizr commented Apr 30, 2021

Copy link
Copy Markdown
Author

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

qqshfox commented Apr 30, 2021

Copy link
Copy Markdown
Contributor

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

nirizr commented Apr 30, 2021

Copy link
Copy Markdown
Author

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