Re-approvals only applied after successful CI run

[benjamb]

Re-approvals only applied after successful CI run

[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!