Test-infra: mungegithub: Proposal: /lgtm also approves

Created on 28 Jan 2017 · 11Comments · Source: kubernetes/test-infra

This is a proposal to improve and simplify the current process. I think it also empowers people:

/lgtm adds LGTM label and approval. This should be the default and most often used command
/approve adds approval only. This is only done if the PR is already lgtm'd and/or approver has no time to review

The idea is that:

/approve means "I have not read the code but I approve the idea" (that meaning doesn't change)
I don't expect people to do /lgtm if the code makes sense but they don't approve the idea anyway
There is no harm in having the reviewer approve the code (even if it doesn't approve any file).
The approver should review the code most of the time, typing just /lgtm will make it easier.

What do you think? @grodrigues3 @philips @bgrant0607 @fejta @kubernetes/sig-contributor-experience-proposals

Source

apelisse

🎉1 👍1

All 11 comments

I like this a lot,

If I made a change it would be to make both /lgtm and /approve be synomyms for /lgtm-approve and create special /approve-only and /lgtm-only for these actions.

it is not clear to me whether this community will consider /approve "I approve the idea but have not read the code" or else "I have read the code and not only do I lgtm it, I approve it." Personally I feel like the latter is closer to how things are used inside Google

fejta on 28 Jan 2017

IOW, your suggestion would be to have:

/approve does LGTM+Approve
/approve-only only approves
/lgtm doesn't exist (I don't think it's needed in this use-case, you shouldn't lgtm if you don't approve)

apelisse on 28 Jan 2017

I have seen that when people are at loggerheads they will frequently do something along the lines of "I do not fully agree with this PR but do not want to block progress on it any further. Please make sure FOO approves." This would be lgtm but not approve

I was thinking something more like:

/lgtm does whatever we think the best default should be (lgtm+approve)
/approve does whatever we think the best default should be (lgtm+approve)
/lgtm-approve does lgtm+approve
/lgtm-only does only lgtm
/approve-only does only approve

fejta on 28 Jan 2017

Specifically I'd love commands to be able to specify each of those three conditions (lgtm only, approve only, lgtm+approve), as well as give people shortcuts (/approve and /lgtm) that allow us to tune what we consider to be good defaults for the community.

fejta on 28 Jan 2017

I really appreciate the intent to streamline for combination reviewers/approvers, but /lgtm and /lgtm-only both existing would be very confusing, in my opinion

liggitt on 28 Jan 2017

it's probably too late to get off the lgtm train, but /reviewed and /approved might be easier to understand if we're trying to distinguish an approval that didn't review the change closely

Edit: on further consideration, /reviewed doesn't necessarily mean the result was that it looked good, so I withdraw the suggestion :)

liggitt on 28 Jan 2017

Yeah, I think my proposed change is reasonable because it creates "the least astonishement". We keep approval exactly as it is, we just overload the meaning of lgtm to also approve. I strongly believe that "lgtm only" doesn't make much sense anyway (or maybe 5% of the time, for which I'm sure we can find a workaround). Yet, I think giving a strong meaning to /lgtm is good because that's what people are used to do. Basically, most people would keep just doing what they were doing before, and that's good because it's already muscle memory.

apelisse on 28 Jan 2017

/lgtm implying approval from an approver makes perfect sense to me.

In the "approved by" comment the bot leaves, linking to the approving comment might close the loop and make it easier to see why the bot considered the PR approved by a particular approver

liggitt on 28 Jan 2017

👍1

SGTM, if I can /LGTM everything that'll work great. Thanks!

fejta on 28 Jan 2017

🎉1

I agree that lgtm should imply approval if performed by an approver.