Prow /approve not cleared on push

[tumido]

Describe the bug
If a contributor force-pushes or pushes into a PR which has an /approve label on it, this label is not cleared - the approve plugin of Prow doesn't support this. This is in contrast with /lgtm where this label get's cleared. See linked issues for details.

This issue is created to raise awareness of this + seek guidance on how we should use the /approve command... Our current setup merges a PR if there's an approve label, no matter if there's an lgtm.

To Reproduce
See: operate-first/operate-first-twitter#10

Expected behavior
Approve label is either removed or the merge is blocked.

Additional context
Upstream bug (closed as WONTFIX): kubernetes/test-infra#17698
operate-first/operate-first-twitter#7 (comment)

[tumido]

@goern it merged the operate-first/operate-first-twitter#10 PR 😕

[goern]

Well, I would go with kubernetes/test-infra#17698 (comment) Is there a reason we want to deviate from the kubernetes behavior/workflow?

/label triage/needs-information

[tumido]

@goern the thing is that we probably don't understand the kubernetes behavior/workflow in detail enough to follow it to point. For me, this behavior is an unpleasant surprise that exposes approved PRs to be exploited to get nonapproved changes into the repo.

This made me think if we have anything to do about this or if we're lacking any documentation in place how we should handle this. I have no idea if there's a code-change required here or rather a policy change or it's just about raising awareness of this and making sure that once a PR is approved it's monitored until merged to make sure there's no commits comming in (which still leaves the loophole exposed, but at least we know about it)...

[tumido]

Since /lgtm gets cleared on commit push, can we make it a requirement for tide to consider a PR for merge? Like configure Tide to merge only if there's both lgtm and approve labels? That would solve the issue I think...

[tumido]

cc @HumairAK @4n4nd @durandom for awareness

[tumido]

attempting to fix this via #1216

[tumido]

It seems the change #1216 worked as intended. At least for operate-first org this is fixed.

Second test PR behaved properly: operate-first/operate-first-twitter#12

[goern]

From our point of view we should follow the Kubernetes workflows as close as possible, this will give us the highest leverage on using Prow. I will close this on for the moment, if some new requirements come out of the ADR creation, feel free to re-open.

/close

[sesheta]

@goern: Closing this issue.

In response to this:

From our point of view we should follow the Kubernetes workflows as close as possible, this will give us the highest leverage on using Prow. I will close this on for the moment, if some new requirements come out of the ADR creation, feel free to re-open.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.