Test-infra: mungegithub: approval-handler: implicit approvals have bad UX

[stevekuznetsov]

When someone in the root OWNERS as an approver makes a PR, it will always have the approved label added immediately. This means that a drive-by reviewer can /lgtm and send the PR off to be merged, even if it's still in a WIP (maybe no tests were added yet so nothing fails). This seems bad. Why should the approved label be implicit and not explicit? Self-approval should still be allowed, but it should be explicit.

/cc @bparees @spxtr @kargakis @eparis
/area mungegithub

[bparees]

big +1, thanks @stevekuznetsov

[stevekuznetsov]

@apelisse @grodrigues3 looks like this was added in https://github.com/kubernetes/test-infra/commit/23d59e951fccaccec01e1fc7415b6ffc5fcdc030 -- thoughts?

[apelisse]

This is something we've discussed a lot when this code was written, and we have considered that use-case/problem at that time.

I do believe that we should make sure WIP code doesn't get merged, rather than remove the auto-approval. Also, I strongly believe that we would lose more time by removing the auto-approval than by having this issue.

[bparees]

I wasn't part of those discussions, but as an outsider just experiencing this model, "Default to merge" seems like an odd choice.

[stevekuznetsov]

@apelisse I think the flow where this bites the most is when a PR is touching code that is verified by non-blocking tests. If someone has triggered non-blocking tests they will not be added to the list of contexts that the submitqueue waits for before merging ( https://github.com/kubernetes/test-infra/issues/3740 ) ... so if you have a WIP PR that is LGTM on the implementation and self-approved, but you're iterating on a non-blocking test, you need to manually add do-not-merge or the submit-queue will happily merge away. If the current behavior seems to be most correct for k8s, I will propose to add a flag to turn the feature off and default it to on.

[grodrigues3]

@stevekuznetsov @apelisse

The purpose of approval was to indicate a high level:

• this idea/change is good for our system and belongs here in this directory.

The bigger problem lies here:

This means that a drive-by reviewer can /lgtm and send the PR off to be merged, even if it's still in a WIP (maybe no tests were added yet so nothing fails).

Code that receives lgtm should be completely ready to be merged (some people assumed it stood for "Let's Get that Merged" :) and it should pass our style guide, have tests written, be passing tests, etc. Perhaps we need to make this clearer: do not /lgtm something unless you want it to get merged.

Initially, lgtm could only be added by a reviewer (someone listed as a reviewer in the OWNERS file) but now anyone in the org can add the label. I'd prefer if we changed the lgtm code rather than the approval code.

[stevekuznetsov]

Fair enough. It still seems like the current system has enough of a gap that a well-intentioned but oblivious developer could get code merged into master that really does not belong there, just because there is no way to opt out of the self-approval. Seems like we should be building an infra that does not allow for simple honest mistakes to require reverts, etc. In my opinion the cleanest way to achieve that would be to make the self-approval still possible but not implicit. If that's too onerous on k8s I would propose making that an option. What do you think?

[apelisse]

If that's too onerous on k8s I would propose making that an option. What do you think?

I'm not opposed to that. Whether we include the author in the list of approvers by default could be flag-configurable. If not activated, then the author could decide to approve when needed.

[grodrigues3]

Sounds fine to me.

• Let's make it configurable.
• Configure it to require explicit approval for k/k
• make an announcement on K8s-dev when it's done.