Re-approvals only applied after successful CI run

[benjamb]

There is a comment related to this within marge/job.py, including for context:

# Re-approve the merge request, in case us pushing it has removed
# approvals. Note that there is a bit of a race; effectively
# approval can't be withdrawn after we've pushed (resetting
# approvals) and CI runs.

Occasionally CI may fail due to transient network issues that are unrelated to the change made. In this case, Marge will error out and not bother attempting to reapply any approvals. GitLab doesn't remove approvals on CI failure, so it doesn't quite make sense that this happens with Marge.

This also applies to any potential exception that might occur between the force push and applying approvals, we need to restart marge and then manually approve again.

I'm unaware as to whether there is a historical reason for why approvals are reapplied when they are, but could they no be applied immediately after the rebase?

[benjamb]

I'd quite like to submit a patch that does something along the lines of:

try:
    push_rebased_and_rewritten_version()
finally:
    apply_approvals()

but I'm unsure as to whether or not that would be appropriate.

[aschmolck]

Right now I can't see why resetting approvals immediately after push wouldn't be OK -- @jcpetruzza? If so I'm happy to move the steps around.

Not sure about the try/finally: if the push went through, the approvals shouldn't have been reset to start with; the case where the push in fact succeeded but there was an exception before confirmation is received seems like it /should/ be exceedingly rare (in which case manually re-approving seems fine). Does this happen to you more often?

[benjamb]

I have a limited understanding of the code base. However, is it not the act of force pushing that is resetting approvals (it is in our case at least), therefore a successful push does not imply approvals haven't been reset.

WRT the use of try/finally: I agree it seems unnecessary, though I've since forgotten why I suggested that :)

[aschmolck]

The approvals are reset on any push, assuming you have configured gitlab to do so (it's optional, but generally useful). Unfortunately gitlab is a bit dumb about resetting approvals and re-running CI: it should probably not do the former and defnitely not the latter if all that changed where merge commit comments.

I'll probably push a fix later this evening (along the lines above).

[benjamb]

@aschmolck I assume that MR was supposed to close #56?

[aschmolck]

@benjamb Indeed :|

[benjamb]

@aschmolck Thanks!