Allow /lgtm to not also mean /approve

[Aaron Crickenberger]

I opened an issue asking for this option to be configurable on a per-repo basis and would appreciate feedback there

https://github.com/kubernetes/test-infra/issues/6589

- aaron

[Aaron Crickenberger]

The comments on the issue were largely in favor of making things configurable or more explicit.  Two folks (who are approvers within k/k) objected.  I raised this issue during contributor experience, and got agreement that we should strive for the principle of least surprise.  I had concerns I was getting agreement from within an echo chamber, so I raised during today's community meeting.  The feedback I got was that we want a consistent process across all repos, but I heard no objections to making it more explicit.

If we're going to enforce the exact same code review process across repos, I'd rather it be as explicit as possible, meaning:

- no implicit self-approval (but allow explicit self-approval)

- explicitly require /approve in order to signify approval (aka modify /lgtm to mean "only apply the lgtm" label)

I'm trying to reach the widest audience of approvers I can.  There are roughly 146 k/k approvers, who I presume understand the subtle intricacies of /lgtm vs /approve.  There are 278 approvers across k/k.  There are 593 members.  I suspect these changes would make it easier to onboard the latter two groups to a consistent code review process across the org.  What do you think?

- aaron

[Clayton Coleman]

Just off the cuff, I would never lgtm something I wouldn’t approve, but I will approve things that I have not looked at except cursorily.  I guess there are a few places where I might lgtm but not want to approve yet, but that’s usually for things in the next release and the milestone is what prevents merging.   I find the pairing convenient but if I end up having to type 8 more characters I guess I won’t suffer too much.

Is there a common reason to lgtm but not approve? 

--
You received this message because you are subscribed to the Google Groups "Kubernetes developer/contributor discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-de...@googlegroups.com.
To post to this group, send email to kuberne...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/6bc1e419-d302-4742-bfcf-ed3fa12ac2c6%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Aaron Crickenberger]

On Thursday, February 15, 2018 at 4:30:03 PM UTC-8, Clayton Coleman wrote:

Is there a common reason to lgtm but not approve? 

My (perhaps unique) example case is that I'm in the root OWNERS file of k/community.  I want to provide a "seems like this is syntactically correct and looks neat" /lgtm but I may not necessarily want to approve on behalf of a SIG that I'm not a member of.  Today I have to issue a /hold, or comment "lgtm" as a human.

- aaron

[Timothy Pepper]

I see the common reason as “teamwork”.

 

Maybe this isn’t quite the right way to say it but “not all reviewers are equal”.  A more positive statement might be “aggregated independent review is highly valuable”.  An /lgtm with a delayed /approve, gives time for multiple additional reviewers to chime in.  Once a sufficient aggregate of /lgtm has been articulated by the review team, such as in the case of the docs folks wanting doc AND tech review or on an issue where there’s some complexity and a need for a number of reviewers, an owner then declares the consensus with /approve.  This is similar to what gerrit based systems do formally or other projects do informally with maintainer delegation hierarchies.  It’s a common model in OSS.  This might give the impression of only incurring latency, but in my experience it turns review and maintainership into a broader team-collaborative act which then over time actually enables faster velocity because there are less human bottlenecks.

 

-- 

Tim Pepper

Open Source Cloud Technologies Lead

VMware Open Source Technology Center

[Tim Hockin]

My personal policy is not to approve things that are not already LGTM
(or that I am not LGTMing at the same time). I guess I don't have
very great reasons for that...

> https://groups.google.com/d/msgid/kubernetes-dev/048b5192-696b-4549-aabd-d5e3e747bdff%40googlegroups.com.

[Daniel Smith]

I sometimes approve things that are not LGTM yet to make sure there is no red tape left when the code has been fixed.

> email to kubernetes-dev+unsubscribe@googlegroups.com.
> To post to this group, send email to kubernetes-dev@googlegroups.com.

> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kubernetes-dev/048b5192-696b-4549-aabd-d5e3e747bdff%40googlegroups.com.
>
> For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Kubernetes developer/contributor discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-dev+unsubscribe@googlegroups.com.
To post to this group, send email to kubernetes-dev@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/CAO_RewbX%2BR4aBxUGc9v-4vmKbDCbSzXj3E9Yfmhst%3DF95doPFw%40mail.gmail.com.

[Brian Grant]

On Thu, Feb 15, 2018 at 4:43 PM, Aaron Crickenberger <spi...@gmail.com> wrote:

On Thursday, February 15, 2018 at 4:30:03 PM UTC-8, Clayton Coleman wrote:

Is there a common reason to lgtm but not approve? 

My (perhaps unique) example case is that I'm in the root OWNERS file of k/community.  I want to provide a "seems like this is syntactically correct and looks neat" /lgtm but I may not necessarily want to approve on behalf of a SIG that I'm not a member of.  Today I have to issue a /hold, or comment "lgtm" as a human.

If I haven't reviewed all aspects of a PR, I don't issue the "/lgtm" command. I just comment about which part I looked at and give an ok. There are an unbounded number of aspects of PRs that specific people may be asked to look at due to their specialized knowledge, especially for larger PRs that touch more aspects of the system: APIs, API machinery, controllers, CLIs, deprecations, new tests, build rules, Salt, bash, ..., so I think it has to be informal rather than automated.

/lgtm has a specific purpose, which is to label the PR as ready for merging once it is approved. If the whole PR isn't ready, nobody should issue /lgtm.

The bot obviously looks at the label, but sometimes humans do, too, such as when they want to find stuck PRs that need approval.

Approval was intended to be a sanity check to facilitate broadening of the reviewer pool.

For a little while, we required assignees to /lgtm, but that became pointless once anyone could assign anyone, so we lost that as an explicit delegation mechanism.

If drive-by /lgtms are a problem, as we speculated they might be when the above change was made, I suggest we fix that rather than make /lgtm meaningless.

 

- aaron

 

On Feb 15, 2018, at 6:52 PM, Aaron Crickenberger <spi...@gmail.com> wrote:

The comments on the issue were largely in favor of making things configurable or more explicit.  Two folks (who are approvers within k/k) objected.  I raised this issue during contributor experience, and got agreement that we should strive for the principle of least surprise.  I had concerns I was getting agreement from within an echo chamber, so I raised during today's community meeting.  The feedback I got was that we want a consistent process across all repos, but I heard no objections to making it more explicit.

If we're going to enforce the exact same code review process across repos, I'd rather it be as explicit as possible, meaning:

- no implicit self-approval (but allow explicit self-approval)

- explicitly require /approve in order to signify approval (aka modify /lgtm to mean "only apply the lgtm" label)

I'm trying to reach the widest audience of approvers I can.  There are roughly 146 k/k approvers, who I presume understand the subtle intricacies of /lgtm vs /approve.  There are 278 approvers across k/k.  There are 593 members.  I suspect these changes would make it easier to onboard the latter two groups to a consistent code review process across the org.  What do you think?

- aaron

On Thursday, February 1, 2018 at 12:40:19 PM UTC-8, Aaron Crickenberger wrote:

I opened an issue asking for this option to be configurable on a per-repo basis and would appreciate feedback there

https://github.com/kubernetes/test-infra/issues/6589

- aaron

--
You received this message because you are subscribed to the Google Groups "Kubernetes developer/contributor discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-de...@googlegroups.com.
To post to this group, send email to kuberne...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/6bc1e419-d302-4742-bfcf-ed3fa12ac2c6%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Kubernetes developer/contributor discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-dev+unsubscribe@googlegroups.com.
To post to this group, send email to kubernetes-dev@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/048b5192-696b-4549-aabd-d5e3e747bdff%40googlegroups.com.

[Brian Grant]

On Thu, Feb 15, 2018 at 4:58 PM, 'Tim Hockin' via Kubernetes developer/contributor discussion <kuberne...@googlegroups.com> wrote:

My personal policy is not to approve things that are not already LGTM
(or that I am not LGTMing at the same time).  I guess I don't have
very great reasons for that...

It used to make sense to /approve in advance once comfortable that the right reviewer was assigned.

Since anyone can /lgtm now, I think it's a good policy to withhold approval.

> email to kubernetes-dev+unsubscribe@googlegroups.com.
> To post to this group, send email to kubernetes-dev@googlegroups.com.

> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kubernetes-dev/048b5192-696b-4549-aabd-d5e3e747bdff%40googlegroups.com.
>
> For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Kubernetes developer/contributor discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-dev+unsubscribe@googlegroups.com.
To post to this group, send email to kubernetes-dev@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/CAO_RewbX%2BR4aBxUGc9v-4vmKbDCbSzXj3E9Yfmhst%3DF95doPFw%40mail.gmail.com.

[Morgan Bauer]

Please define LGTM and approve. I do not understand the difference and meanings in the kubernetes context.

To me LGTM means "ready to merge, immediately". While I do not recall cases in the core kube repos, I've seen LGTM used as a carrot, as in "you have my LGTM if you frob the baz instead, and add a comment".

I am not sure what approve could mean. Does it mean "I like the idea"? I've seen things like SGTM (Sounds GTM) in the contexts of "I like the idea, but I don't have the expertise to comment", or "I haven't reviewed the details, but the description makes sense".

I'd love to help move things along any way I can.

--

[Tim Hockin]

LGTM means "I reviewed this in detail and I am OK with it merging".
We don't have a partial LGTM, but we do have partial approval.

Approve means "I have authority over some or all of the domain of this
PR and I am satisfied with it (or with the reviewer(s) that LGTM'ed
it)".


To me, approval lets me retain some ability to sanity-check, while
reviewers build "cred". At some point, those people should get the
ability to approve on their project areas and I won't be needed.

There are also a small number of areas of the project that are
delicate enough to warrant an extra level of control - godeps, for
example. Because of licensing issues, that necessarily will stay
smaller than other areas, for now.

When code changes (push) LGTM is auto-removed. Perhaps approval should be, too.

Nothing makes me happier than opening my PR queue to find 25 PRs that
are already LGTM'ed and just waiting for approval.

> https://groups.google.com/d/msgid/kubernetes-dev/dd384570-9ae4-f423-75e3-00f4fd8a7edd%40gmail.com.

[Brian Grant]

On Fri, Feb 16, 2018 at 8:44 AM, 'Tim Hockin' via Kubernetes developer/contributor discussion <kuberne...@googlegroups.com> wrote:

LGTM means "I reviewed this in detail and I am OK with it merging".
We don't have a partial LGTM, but we do have partial approval.

Approve means "I have authority over some or all of the domain of this
PR and I am satisfied with it (or with the reviewer(s) that LGTM'ed
it)".


To me, approval lets me retain some ability to sanity-check, while
reviewers build "cred".  At some point, those people should get the
ability to approve on their project areas and I won't be needed.

There are also a small number of areas of the project that are
delicate enough to warrant an extra level of control - godeps, for
example.  Because of licensing issues, that necessarily will stay
smaller than other areas, for now.

I agree with all of the above.

 

When code changes (push) LGTM is auto-removed.  Perhaps approval should be, too.

Sadly, that may be necessary with the way /lgtm works now.

 

> email to kubernetes-dev+unsubscribe@googlegroups.com.
> To post to this group, send email to kubernetes-dev@googlegroups.com.

> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kubernetes-dev/6bc1e419-d302-4742-bfcf-ed3fa12ac2c6%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Kubernetes developer/contributor discussion" group.
> To unsubscribe from this group and stop receiving emails from it, send an

> email to kubernetes-dev+unsubscribe@googlegroups.com.
> To post to this group, send email to kubernetes-dev@googlegroups.com.

> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kubernetes-dev/dd384570-9ae4-f423-75e3-00f4fd8a7edd%40gmail.com.
>
> For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Kubernetes developer/contributor discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-dev+unsubscribe@googlegroups.com.
To post to this group, send email to kubernetes-dev@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/CAO_RewZoa9dzGF0SVPkYcMjf4f%3DPV%2Bg%3DoYRfjqke%2BTm5hNABHg%40mail.gmail.com.

[Clayton Coleman]

I really don't want to have to repeatedly approve things after the general direction is agreed.  That would be a significant step backwards.

On Fri, Feb 16, 2018 at 11:59 AM, 'Brian Grant' via Kubernetes developer/contributor discussion <kuberne...@googlegroups.com> wrote:

> To post to this group, send email to kuberne...@googlegroups.com.

> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kubernetes-dev/6bc1e419-d302-4742-bfcf-ed3fa12ac2c6%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Kubernetes developer/contributor discussion" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kubernetes-dev+unsubscribe@googlegroups.com.

> To post to this group, send email to kuberne...@googlegroups.com.

> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kubernetes-dev/dd384570-9ae4-f423-75e3-00f4fd8a7edd%40gmail.com.
>
> For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Kubernetes developer/contributor discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-dev+unsubscribe@googlegroups.com.

To post to this group, send email to kuberne...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/CAO_RewZoa9dzGF0SVPkYcMjf4f%3DPV%2Bg%3DoYRfjqke%2BTm5hNABHg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Kubernetes developer/contributor discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-dev+unsubscribe@googlegroups.com.
To post to this group, send email to kubernetes-dev@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/CAKCBhs7AKUmL33U-WjM_X08owhU2Q6mEmmm%3DKhJLVT8ewFwJXw%40mail.gmail.com.

[Daniel Smith]

On Fri, Feb 16, 2018 at 7:26 AM, 'Brian Grant' via Kubernetes developer/contributor discussion <kuberne...@googlegroups.com> wrote:

On Thu, Feb 15, 2018 at 4:58 PM, 'Tim Hockin' via Kubernetes developer/contributor discussion <kubernetes-dev@googlegroups.com> wrote:

My personal policy is not to approve things that are not already LGTM
(or that I am not LGTMing at the same time).  I guess I don't have
very great reasons for that...

It used to make sense to /approve in advance once comfortable that the right reviewer was assigned.

Since anyone can /lgtm now, I think it's a good policy to withhold approval.

O_o Well, I guess I have to stop doing that, then...

 

> https://groups.google.com/d/msgid/kubernetes-dev/048b5192-696b-4549-aabd-d5e3e747bdff%40googlegroups.com.
>
> For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Kubernetes developer/contributor discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-dev+unsubscribe@googlegroups.com.

To post to this group, send email to kuberne...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/CAO_RewbX%2BR4aBxUGc9v-4vmKbDCbSzXj3E9Yfmhst%3DF95doPFw%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Kubernetes developer/contributor discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-dev+unsubscribe@googlegroups.com.
To post to this group, send email to kubernetes-dev@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/CAKCBhs7CYRQw9O22XD%3Dzfj4LWixfxjjus8L1V7BORVXN7PGHog%40mail.gmail.com.

[Tim Hockin]

Caleb keep chiding me for trying to hold too much control over things
that don't need it. I understand his points, but it's hard to
overcome...

>>> > https://groups.google.com/d/msgid/kubernetes-dev/048b5192-696b-4549-aabd-d5e3e747bdff%40googlegroups.com.
>>> >
>>> > For more options, visit https://groups.google.com/d/optout.
>>>
>>> --
>>> You received this message because you are subscribed to the Google Groups
>>> "Kubernetes developer/contributor discussion" group.
>>> To unsubscribe from this group and stop receiving emails from it, send an

>>> email to kubernetes-de...@googlegroups.com.
>>> To post to this group, send email to kuberne...@googlegroups.com.
>>> To view this discussion on the web visit

>>> https://groups.google.com/d/msgid/kubernetes-dev/CAO_RewbX%2BR4aBxUGc9v-4vmKbDCbSzXj3E9Yfmhst%3DF95doPFw%40mail.gmail.com.
>>> For more options, visit https://groups.google.com/d/optout.
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Kubernetes developer/contributor discussion" group.
>> To unsubscribe from this group and stop receiving emails from it, send an

>> email to kubernetes-de...@googlegroups.com.
>> To post to this group, send email to kuberne...@googlegroups.com.
>> To view this discussion on the web visit

[Aaron Crickenberger]

So now I've managed to hear from people in kubernetes/kubernetes/OWNERS but not really the rest of the project.  The silence gives me no signal on what the majority of the project wants (https://en.wikipedia.org/wiki/Warnock%27s_dilemma)

I'm going to posit that given the feedback I've gotten from newcomers, it wouldn't hurt to be more explicit with our commands that apply these two labels.  I'd like to move ahead with PR's to configure as such, meaning:

- no implicit self-approval (but allow explicit self-approval)

- explicitly require /approve in order to signify approval (aka modify /lgtm to mean "only apply the lgtm" label)

Please object if I'm headed in the wrong direction.

- aaron

[Clayton Coleman]

I’m ok with trying it out.  

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/0e9e76ed-c890-4f62-8178-42229bd7e628%40googlegroups.com.

[Christoph Blecker]

I explicitly support this direction.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-dev+unsubscribe@googlegroups.com.
To post to this group, send email to kubernetes-dev@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/0e9e76ed-c890-4f62-8178-42229bd7e628%40googlegroups.com.

[Tim Hockin]

+1

>> https://groups.google.com/d/msgid/kubernetes-dev/0e9e76ed-c890-4f62-8178-42229bd7e628%40googlegroups.com.
>>
>> For more options, visit https://groups.google.com/d/optout.
>
>

> --
> You received this message because you are subscribed to the Google Groups
> "Kubernetes developer/contributor discussion" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kubernetes-de...@googlegroups.com.
> To post to this group, send email to kuberne...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/kubernetes-dev/CADx2oGFCOFJ%3D6eA%2BkMoDLaX71qkkYXMGT9wGh0CtvUbSEsD92Q%40mail.gmail.com.