Decouple /lgtm and /approve (important especially for top level approvers)

378 views
Skip to first unread message

Davanum Srinivas

unread,
Sep 26, 2018, 10:26:35 AM9/26/18
to kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
Team,

Please take a look at the change proposed in:

Text from the PR:

Currently folks who are in the top level OWNERS files have an issue when the use /lgtm. As currently configured the bots assume that the /lgtm is the same as /approve and end up merging PRs. Sometimes, the OWNERS want to just say (for example) that this looks good, but go ask others for approvals. So let's explicitly require them to specify their approval when they wish and not confuse it with a LGTM.

Does this look good? Please chime in the PR above. 

Let's use lazy consensus at least a week (may be merge Oct 5 if there are no issues).

Thanks,
Dims

--
Davanum Srinivas :: https://twitter.com/dims

Hemant Kumar

unread,
Sep 26, 2018, 10:36:03 AM9/26/18
to dav...@gmail.com, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
+1 to this. I often put a PR on hold after lgtming to make sure someone else had a chance to review before it gets merged but it is easy to forget and a PR can get merged by just one person's lgtm/approve which is perhaps not what we want?



--
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/CANw6fcF242-LgCXJmrgjzpf-wUWOFo8E%3DSxDpuMF%2BWu4-Gc4Dw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Tim Hockin

unread,
Sep 26, 2018, 11:25:27 AM9/26/18
to Hemant Kumar, Davanum Srinivas, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
Agree agree agree - there should be no relationship in either
direction. They are orthogonal actions, in my book.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/CAHTCLtDD-0F%3D7dyC632jCUvGfHKq93niwQp1zjjkjAd3JNd5uA%40mail.gmail.com.

Timothy Pepper

unread,
Sep 26, 2018, 11:30:36 AM9/26/18
to Tim Hockin, Hemant Kumar, Davanum Srinivas, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
Since we're talking about this again, I'd again like to suggest considering making the commands and associated labels more directly indicate intent, for example:

/review ok
/review changes-requested
/approved-for-merge

The first two arguably map to "/lgtm" and "", but the latter is obviously imprecise.

A change like this would be a useful improvement for new contributor experience, overall precision of the review process, and its associated artifacts.

--
Tim Pepper
Orchestration & Containers Lead
VMware Open Source Technology Center

On 9/26/18, 8:25 AM, "'Tim Hockin' via Kubernetes developer/contributor discussion" <kuberne...@googlegroups.com> wrote:

Agree agree agree - there should be no relationship in either
direction. They are orthogonal actions, in my book.
On Wed, Sep 26, 2018 at 7:36 AM Hemant Kumar <hek...@redhat.com> wrote:
>
> +1 to this. I often put a PR on hold after lgtming to make sure someone else had a chance to review before it gets merged but it is easy to forget and a PR can get merged by just one person's lgtm/approve which is perhaps not what we want?
>
>
>
> On Wed, Sep 26, 2018 at 10:26 AM Davanum Srinivas <dav...@gmail.com> wrote:
>>
>> Team,
>>
>> Please take a look at the change proposed in:
>> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fkubernetes%2Ftest-infra%2Fpull%2F9151&amp;data=02%7C01%7Ctpepper%40vmware.com%7Caccd0ecb1c31413b050e08d623c4488a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636735723268304112&amp;sdata=xjq%2BoTa6TpSoH4Y2v9W1vwJ4Wd1zGArKgOgMtvi5jzw%3D&amp;reserved=0
>>
>> Text from the PR:
>>
>> Currently folks who are in the top level OWNERS files have an issue when the use /lgtm. As currently configured the bots assume that the /lgtm is the same as /approve and end up merging PRs. Sometimes, the OWNERS want to just say (for example) that this looks good, but go ask others for approvals. So let's explicitly require them to specify their approval when they wish and not confuse it with a LGTM.
>>
>> Does this look good? Please chime in the PR above.
>>
>> Let's use lazy consensus at least a week (may be merge Oct 5 if there are no issues).
>>
>> Thanks,
>> Dims
>>
>> --
>> Davanum Srinivas :: https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftwitter.com%2Fdims&amp;data=02%7C01%7Ctpepper%40vmware.com%7Caccd0ecb1c31413b050e08d623c4488a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636735723268304112&amp;sdata=KEfycUD9FPO1l4%2FDgMhJdbFrMA1CIc3qcoqcVa0szOo%3D&amp;reserved=0
>>
>> --
>> 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://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fkubernetes-dev%2FCANw6fcF242-LgCXJmrgjzpf-wUWOFo8E%253DSxDpuMF%252BWu4-Gc4Dw%2540mail.gmail.com&amp;data=02%7C01%7Ctpepper%40vmware.com%7Caccd0ecb1c31413b050e08d623c4488a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636735723268304112&amp;sdata=fIe1VL46HXJdzQP0Jco6aCsF%2BiQn%2BKZpxu0cjeryZP8%3D&amp;reserved=0.
>> For more options, visit https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Foptout&amp;data=02%7C01%7Ctpepper%40vmware.com%7Caccd0ecb1c31413b050e08d623c4488a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636735723268304112&amp;sdata=iqPw7zbLOmSizWAhC34sL6PJi4cUzR7aTztqOT%2FeT8M%3D&amp;reserved=0.
>
> --
> 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://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fkubernetes-dev%2FCAHTCLtDD-0F%253D7dyC632jCUvGfHKq93niwQp1zjjkjAd3JNd5uA%2540mail.gmail.com&amp;data=02%7C01%7Ctpepper%40vmware.com%7Caccd0ecb1c31413b050e08d623c4488a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636735723268304112&amp;sdata=UT8E%2Fss8SSay1M1Y5FrK%2FOE2BV2k0uxntSe76d5jyfI%3D&amp;reserved=0.
> For more options, visit https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Foptout&amp;data=02%7C01%7Ctpepper%40vmware.com%7Caccd0ecb1c31413b050e08d623c4488a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636735723268304112&amp;sdata=iqPw7zbLOmSizWAhC34sL6PJi4cUzR7aTztqOT%2FeT8M%3D&amp;reserved=0.

--
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://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fkubernetes-dev%2FCAO_RewZCvTg4m1XLoiEFKQXg4erVkKcYt3c0YHVvc2trWg7jQw%2540mail.gmail.com&amp;data=02%7C01%7Ctpepper%40vmware.com%7Caccd0ecb1c31413b050e08d623c4488a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636735723268304112&amp;sdata=OV463M%2BMuAE37RbGICQ5Sf4NMzYHGjqFSV%2B63erNJqI%3D&amp;reserved=0.
For more options, visit https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Foptout&amp;data=02%7C01%7Ctpepper%40vmware.com%7Caccd0ecb1c31413b050e08d623c4488a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636735723268304112&amp;sdata=iqPw7zbLOmSizWAhC34sL6PJi4cUzR7aTztqOT%2FeT8M%3D&amp;reserved=0.


Tim Hockin

unread,
Sep 26, 2018, 12:01:37 PM9/26/18
to Tim Pepper, Hemant Kumar, Davanum Srinivas, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
Github's review system kind of stinks, but doesn't it mostly represent
this anyway?

Clayton Coleman

unread,
Sep 26, 2018, 12:05:17 PM9/26/18
to tpe...@vmware.com, Hockin, Tim, Hemant Kumar, Davanum Srinivas, kubernetes-s...@googlegroups.com, kubernetes-dev
All three of those are a lot more complicated to remember and explain to a new user - it's really hard for me to believe that explaining those is easier than "if you're ok with this, say /lgtm, if you approve it, say /approve".

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/69E08293-7444-48AF-A8A7-CA00BE64C45E%40vmware.com.
For more options, visit https://groups.google.com/d/optout.

Brendan Burns

unread,
Sep 26, 2018, 12:12:00 PM9/26/18
to tpe...@vmware.com, Clayton Coleman, Hockin, Tim, Hemant Kumar, Davanum Srinivas, kubernetes-s...@googlegroups.com, kubernetes-dev
I'm ok with /lgtm != /approve, but I kind of feel like /approve implies /lgtm.

Or at least /approve implies "ready to merge" which implies /lgtm.

I'm 100% with Clayton that a new larger set of labels isn't going to improve things...

--brendan

From: kuberne...@googlegroups.com <kuberne...@googlegroups.com> on behalf of Clayton Coleman <ccol...@redhat.com>
Sent: Wednesday, September 26, 2018 9:05:02 AM
To: tpe...@vmware.com
Cc: Hockin, Tim; Hemant Kumar; Davanum Srinivas; kubernetes-s...@googlegroups.com; kubernetes-dev
Subject: Re: Decouple /lgtm and /approve (important especially for top level approvers)
 

Tim Hockin

unread,
Sep 26, 2018, 12:15:16 PM9/26/18
to Brendan Burns, Tim Pepper, Clayton Coleman, Hemant Kumar, Davanum Srinivas, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
I want /approve to NOT imply /lgtm.

I get PRs that touch deps or top-level files or many things that have
limited approvers. I want to be able to approve those things without
LGTM'ing the whole PR.

Quinton Hoole

unread,
Sep 26, 2018, 12:19:53 PM9/26/18
to Tim Hockin, Brendan Burns, Timothy Pepper, Clayton Coleman, hek...@redhat.com, Davanum Srinivas, kubernetes-s...@googlegroups.com, kubernetes-dev
I agree with Tim, and the main proposal.  In my mind /lgtm implies that I have reviewed the whole PR.  /approve implies that I have verified that the right set of people have reviewed the PR adequately, and in my judgement it's good to go (even if I have not reviewed it myself).

Q


You received this message because you are subscribed to the Google Groups "kubernetes-sig-contribex" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-con...@googlegroups.com.
To post to this group, send email to kubernetes-s...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-contribex/CAO_RewazinRyc-hAfqQE2Xkt4A3KVUFhW3xBO-40BUpPxnJxJw%40mail.gmail.com.

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


--
Quinton Hoole
qui...@hoole.biz

Brendan Burns

unread,
Sep 26, 2018, 12:20:30 PM9/26/18
to Tim Hockin, Tim Pepper, Clayton Coleman, Hemant Kumar, Davanum Srinivas, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
Yeah I totally get that. But I feel like on that case the right flow is to wait for someone else to /lgtm and then you /approve.

So given that you'd never /approve before all of the /lgtms exist, if you do /approve before /lgtm I feel like /approve by itself is short hand for:

/lgtm
/approve

But given that it only saves six characters I don't really care...

--brendan

From: Tim Hockin <tho...@google.com>
Sent: Wednesday, September 26, 2018 9:14:59 AM
To: Brendan Burns
Cc: Tim Pepper; Clayton Coleman; Hemant Kumar; Davanum Srinivas; kubernetes-s...@googlegroups.com; Kubernetes developer/contributor discussion

>>     >>
>>     >> Text from the PR:
>>     >>
>>     >> Currently folks who are in the top level OWNERS files have an issue when the use /lgtm. As currently configured the bots assume that the /lgtm is the same as /approve and end up merging PRs. Sometimes, the OWNERS want to just say (for example) that this looks good, but go ask others for approvals. So let's explicitly require them to specify their approval when they wish and not confuse it with a LGTM.
>>     >>
>>     >> Does this look good? Please chime in the PR above.
>>     >>
>>     >> Let's use lazy consensus at least a week (may be merge Oct 5 if there are no issues).
>>     >>
>>     >> Thanks,
>>     >> Dims
>>     >>
>>     >> --

>>     >>
>>     >> --
>>     >> 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://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fkubernetes-dev%2FCANw6fcF242-LgCXJmrgjzpf-wUWOFo8E%253DSxDpuMF%252BWu4-Gc4Dw%2540mail.gmail.com&amp;data=02%7C01%7Cbburns%40microsoft.com%7Ca1545e143dbc4451e54608d623cb3ce2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636735753140302173&amp;sdata=LD28ZjzCJrHEemV7OVLfzSIDHIc%2F3yh37RHQ9iijyMg%3D&amp;reserved=0.
>>     >> For more options, visit https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Foptout&amp;data=02%7C01%7Cbburns%40microsoft.com%7Ca1545e143dbc4451e54608d623cb3ce2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636735753140302173&amp;sdata=7m%2FWs6t8abNZLq%2BiFQ2Ja5T7IhZRqG1OW4U1FfOdvkw%3D&amp;reserved=0.

>>     >
>>     > --
>>     > 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://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fkubernetes-dev%2FCAHTCLtDD-0F%253D7dyC632jCUvGfHKq93niwQp1zjjkjAd3JNd5uA%2540mail.gmail.com&amp;data=02%7C01%7Cbburns%40microsoft.com%7Ca1545e143dbc4451e54608d623cb3ce2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636735753140302173&amp;sdata=HEEIe4CO%2FnxsXd1ij0XRyb2XZt5xlwsd5Gim621U32A%3D&amp;reserved=0.
>>     > For more options, visit https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Foptout&amp;data=02%7C01%7Cbburns%40microsoft.com%7Ca1545e143dbc4451e54608d623cb3ce2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636735753140302173&amp;sdata=7m%2FWs6t8abNZLq%2BiFQ2Ja5T7IhZRqG1OW4U1FfOdvkw%3D&amp;reserved=0.

>>
>>     --
>>     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://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fkubernetes-dev%2FCAO_RewZCvTg4m1XLoiEFKQXg4erVkKcYt3c0YHVvc2trWg7jQw%2540mail.gmail.com&amp;data=02%7C01%7Cbburns%40microsoft.com%7Ca1545e143dbc4451e54608d623cb3ce2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636735753140312177&amp;sdata=YEBZR%2BlM4GAIbK3g0suiAfMHePpcFgptWYhg0k%2FJkGk%3D&amp;reserved=0.
>>     For more options, visit https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Foptout&amp;data=02%7C01%7Cbburns%40microsoft.com%7Ca1545e143dbc4451e54608d623cb3ce2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636735753140312177&amp;sdata=nrTUEe56eh6R0wu76MbulVdiLAy2mipk%2B%2Bc3MeMD2g8%3D&amp;reserved=0.

>>
>>
>> --
>> 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.

>
> --
> 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.

Tim Hockin

unread,
Sep 26, 2018, 12:23:24 PM9/26/18
to Brendan Burns, Tim Pepper, Clayton Coleman, Hemant Kumar, Davanum Srinivas, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
"wait for someone else to LGTM" means, in my experience, PR author
waits an unpredictable amount of time for me to get back to a PR to
check if it was LGTM'ed. We just don't have the notifications needed
to make that work well.

I would accept `/approve <path> <path> <path>` or somethign, but that
is a lot more work.
On Wed, Sep 26, 2018 at 9:20 AM 'Brendan Burns' via Kubernetes
> To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/CY4PR21MB05042A8FCCB5C7899EF1FC05DB150%40CY4PR21MB0504.namprd21.prod.outlook.com.
> For more options, visit https://groups.google.com/d/optout.

Timothy Pepper

unread,
Sep 26, 2018, 12:30:44 PM9/26/18
to Clayton Coleman, Hockin, Tim, Hemant Kumar, Davanum Srinivas, kubernetes-s...@googlegroups.com, kubernetes-dev

It feels like the discussion here in the thread strongly implies even senior people have trouble explaining the “/lgtm” to senior people.  Are you sure it’s easy to explain to a new user?

 

-- 

Tim Pepper

Orchestration & Containers Lead

VMware Open Source Technology Center

 

From: <kuberne...@googlegroups.com> on behalf of Clayton Coleman <ccol...@redhat.com>
Date: Wednesday, September 26, 2018 at 9:05 AM
To: Timothy Pepper <tpe...@vmware.com>
Cc: "Hockin, Tim" <tho...@google.com>, Hemant Kumar <hek...@redhat.com>, Davanum Srinivas <dav...@gmail.com>, "kubernetes-s...@googlegroups.com" <kubernetes-s...@googlegroups.com>, kubernetes-dev <kuberne...@googlegroups.com>
Subject: Re: Decouple /lgtm and /approve (important especially for top level approvers)

 

All three of those are a lot more complicated to remember and explain to a new user - it's really hard for me to believe that explaining those is easier than "if you're ok with this, say /lgtm, if you approve it, say /approve".

Daniel Smith

unread,
Sep 26, 2018, 12:40:41 PM9/26/18
to Tim Hockin, Brendan Burns, Timothy Pepper, Clayton Coleman, hek...@redhat.com, Davanum Srinivas, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
Yeah, let me +1 the idea that lgtm and approve are orthogonal, also that we should not change the commands.

lgtm = this code appears free of bugs / style problems / accomplishes its stated purpose
approve = the goal of the PR is aligned with the Kubernetes project direction, and the high level structure is OK (e.g., package dependencies all go the right direction)

I generally don't /approve until the PR is in the "one last round of nits" stage, but it's important for the overall workflow that I be able to approve PRs that still need a rebase or a nit fixed or something: otherwise, I have to give the /approval on the merge queue's timeline instead of when I have a timeslice for reviewing.

You received this message because you are subscribed to the Google Groups "kubernetes-sig-contribex" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-con...@googlegroups.com.
To post to this group, send email to kubernetes-s...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-contribex/CAO_RewYsy_LANoDZ7V33Wp5qexgr6vXU%2B5rge0E074nLGwBJDA%40mail.gmail.com.

Tim Hockin

unread,
Sep 26, 2018, 12:41:36 PM9/26/18
to Tim Pepper, Clayton Coleman, Hemant Kumar, Davanum Srinivas, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
I don't think that the implication of this thread, we're just debating
(based on our own experiences) how we use the tool in hand.

Stephen Augustus

unread,
Sep 26, 2018, 12:45:56 PM9/26/18
to dbs...@google.com, tho...@google.com, Brendan Burns, Timothy Pepper, Clayton Coleman, hek...@redhat.com, Davanum Srinivas, kubernetes-s...@googlegroups.com, kuberne...@googlegroups.com
Agreed that I think the current tools solve what we're trying to accomplish; lgtm and approve just needed to be decoupled for the repo first.

As a top-level approver...

Approve all the things:
/lgtm
/approve

Just approve:
/approve

Approve the intent, but not lgtm (for a repo that doesn't have the two decoupled):
/hold
/approve

The latter doesn't solve the auto-lgtm for a repo that doesn't have the two decoupled, but it does prevent the auto-merge.

-- Stephen

Josh Berkus

unread,
Sep 26, 2018, 1:18:19 PM9/26/18
to Quinton Hoole, Tim Hockin, Brendan Burns, Timothy Pepper, Clayton Coleman, hek...@redhat.com, Davanum Srinivas, kubernetes-s...@googlegroups.com, kubernetes-dev
On 09/26/2018 09:19 AM, Quinton Hoole wrote:
> I agree with Tim, and the main proposal.  In my mind /lgtm implies that
> I have reviewed the whole PR.  /approve implies that I have verified
> that the right set of people have reviewed the PR adequately, and in my
> judgement it's good to go (even if I have not reviewed it myself).
>

The problem is that what /approve means and what /lgtm means depends on
which repository/SIG you're dealing with.

For some teams:
/lgtm is equivalent to +1
/approve is "merge it"

For other teams:
/approve is "meets standards, OK to merge if LGTM'd"
/lgtm is "merge it"

This makes it difficult to maintain community standards for how the two
labels are to be used, or to train new contributors. We don't need to
change labels as much as we need to make practices consistent. If that
involves changing labels, I'm for it, but changing labels alone won't
fix anything.

--
--
Josh Berkus
Kubernetes Community
Red Hat OSAS

Aaron Crickenberger

unread,
Sep 26, 2018, 1:31:55 PM9/26/18
to Kubernetes developer/contributor discussion
I cannot +1 Dims' proposal enthusiastically enough

- aaron

Tim Hockin

unread,
Sep 26, 2018, 1:46:30 PM9/26/18
to Aaron Crickenberger, Kubernetes developer/contributor discussion
The more I read this thread, the more I want `/lgtm <path> <path>
<path>` and `/approve <path> <path> <path>` - I WANT lgtm and approval
to be driven by non-top-level owners primarily, and I do NOT want to
be giving tacit approval to things I didn't really process
sufficiently.

In fact, what I really want is to delegate approval. I want to `/lgtm
Godeps/... vendor/...` and `/approve-by @other-person`. There's no
reason that most PRs need my approval, but practically they do if they
touch deps or build or ...
> --
> 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/07c63405-ef3e-4c47-a493-61c2894cca99%40googlegroups.com.

Benjamin Elder

unread,
Sep 26, 2018, 1:47:26 PM9/26/18
to jbe...@redhat.com, qui...@hoole.biz, Tim Hockin, bbu...@microsoft.com, Tim Pepper, ccol...@redhat.com, hek...@redhat.com, Davanum Srinivas, kubernetes-s...@googlegroups.com, kubernetes-dev
If this goes well we should consider trying to move all repos and orgs to the same settings for lgtm + approve.

I support this change. adding an explicit lgtm to an approval comment is only a few more characters to type :^)

--
You received this message because you are subscribed to the Google Groups "kubernetes-sig-contribex" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-con...@googlegroups.com.
To post to this group, send email to kubernetes-s...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-contribex/345a5733-aa3d-f6b6-8251-969a5d3ef029%40redhat.com.

Davanum Srinivas

unread,
Sep 26, 2018, 1:57:40 PM9/26/18
to benth...@google.com, jbe...@redhat.com, qui...@hoole.biz, tho...@google.com, bbu...@microsoft.com, tpe...@vmware.com, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
Scenario #1 : "Oh my god! what did i do?, i merged the darn thing"
Scenario #2 : "Hmm, this thing did not merge, darn i just did the /lgtm, let me throw in /approve too"

In the PR reference, i just want to go from #1 to #2. All the good ideas about things we could do from this thread would be a follow up. 

Sounds good?

Thanks,
Dims

Antoine Pelisse

unread,
Sep 26, 2018, 2:00:55 PM9/26/18
to dav...@gmail.com, Benjamin Elder, Josh Berkus, qui...@hoole.biz, Tim Hockin, bbu...@microsoft.com, tpe...@vmware.com, Clayton Coleman, hek...@redhat.com, kubernetes-s...@googlegroups.com, kubernetes-dev
On Wed, Sep 26, 2018 at 10:57 AM Davanum Srinivas <dav...@gmail.com> wrote:
Scenario #1 : "Oh my god! what did i do?, i merged the darn thing"
Scenario #2 : "Hmm, this thing did not merge, darn i just did the /lgtm, let me throw in /approve too"

In the PR reference, i just want to go from #1 to #2. All the good ideas about things we could do from this thread would be a follow up. 

Sounds good?

/lgtm
/approve 

Jordan Liggitt

unread,
Sep 26, 2018, 2:21:59 PM9/26/18
to Davanum Srinivas, benth...@google.com, jbe...@redhat.com, qui...@hoole.biz, tho...@google.com, bbu...@microsoft.com, tpe...@vmware.com, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion

> On Sep 26, 2018, at 10:57 AM, Davanum Srinivas <dav...@gmail.com> wrote:
>
> Scenario #2 : "Hmm, this thing did not merge, darn i just did the /lgtm, let me throw in /approve too"
>
> In the PR reference, i just want to go from #1 to #2. All the good ideas about things we could do from this thread would be a follow up.

I agree we don't want accidental merges, but the friction/lag for hunting down one or more approvers a second time can be significant. Not to say we shouldn't make the change, but we should be aware of the impact on people and velocity.

> The more I read this thread, the more I want `/lgtm <path> <path>
> <path>` and `/approve <path> <path> <path>`

I agree with Tim about wanting the ability to approve or lgtm a relevant portion of a PR.

Brendan Burns

unread,
Sep 26, 2018, 2:42:49 PM9/26/18
to Jordan Liggitt, Davanum Srinivas, benth...@google.com, jbe...@redhat.com, qui...@hoole.biz, tho...@google.com, tpe...@vmware.com, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
If we go down the path based approach (which is fine, imho) we're definitely going to have to improve the tooling to print out what /approve or /lgtm paths are missing.

something like:

/status

which returns:

"/lgtm is missing for /foo, /approve is missing for /bar/baz"

Otherwise it's going to be really hard for anyone to understand what is needed to get a PR merged...

And regarding Tim's point, I think "notification for ready to approve or review" is orthogonal to /lgtm or /approve.  I 100% agree it should be better though...


From: Jordan Liggitt <jor...@liggitt.net>
Sent: Wednesday, September 26, 2018 11:21 AM
To: Davanum Srinivas
Cc: benth...@google.com; jbe...@redhat.com; qui...@hoole.biz; tho...@google.com; Brendan Burns; tpe...@vmware.com; ccol...@redhat.com; hek...@redhat.com; kubernetes-s...@googlegroups.com; Kubernetes developer/contributor discussion

Subject: Re: Decouple /lgtm and /approve (important especially for top level approvers)

Davanum Srinivas

unread,
Sep 26, 2018, 3:05:12 PM9/26/18
to bbu...@microsoft.com, jor...@liggitt.net, benth...@google.com, jbe...@redhat.com, qui...@hoole.biz, tho...@google.com, tpe...@vmware.com, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
Brendan,

Here's what Tide status looks like in a typical PR.

"Not mergeable. Should not have do-not-merge/cherry-pick-not-approved label."

Is this good enough (or updating it) without adding a status command?

Thanks,
Dims

Brendan Burns

unread,
Sep 26, 2018, 3:57:24 PM9/26/18
to Davanum Srinivas, jor...@liggitt.net, benth...@google.com, jbe...@redhat.com, qui...@hoole.biz, tho...@google.com, tpe...@vmware.com, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
As long as we improve the status to include paths, that's fine with me. I just want to make sure that we cover both the "approve limited paths" and "view which paths need to be approved" use cases :)

Thanks!


From: Davanum Srinivas <dav...@gmail.com>
Sent: Wednesday, September 26, 2018 12:04 PM
To: Brendan Burns
Cc: jor...@liggitt.net; benth...@google.com; jbe...@redhat.com; qui...@hoole.biz; tho...@google.com; tpe...@vmware.com; ccol...@redhat.com; hek...@redhat.com; kubernetes-s...@googlegroups.com; Kubernetes developer/contributor discussion

Timothy Pepper

unread,
Sep 26, 2018, 4:16:38 PM9/26/18
to Brendan Burns, Davanum Srinivas, jor...@liggitt.net, benth...@google.com, jbe...@redhat.com, qui...@hoole.biz, tho...@google.com, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion

I’m interested to know more how often top level folks want to “approve limited paths” and if there’s a pattern there which argues for OWNERS files updates to make delegation formal.  For deps, should vendor/OWNERS have some filters applied instead of just:

approvers:

- dep-approvers

 

The “view which paths need to be approved” case is covered today.   Not sure how to do this durably aside from screen shotting…

 

 

 

-- 

Tim Pepper

Orchestration & Containers Lead

VMware Open Source Technology Center

 

From: Brendan Burns <bbu...@microsoft.com>
Date: Wednesday, September 26, 2018 at 12:57 PM
To: Davanum Srinivas <dav...@gmail.com>
Cc: "jor...@liggitt.net" <jor...@liggitt.net>, "benth...@google.com" <benth...@google.com>, "jbe...@redhat.com" <jbe...@redhat.com>, "qui...@hoole.biz" <qui...@hoole.biz>, "tho...@google.com" <tho...@google.com>, Timothy Pepper <tpe...@vmware.com>, "ccol...@redhat.com" <ccol...@redhat.com>, "hek...@redhat.com" <hek...@redhat.com>, "kubernetes-s...@googlegroups.com" <kubernetes-s...@googlegroups.com>, Kubernetes developer/contributor discussion <kuberne...@googlegroups.com>
Subject: Re: Decouple /lgtm and /approve (important especially for top level approvers)

 

As long as we improve the status to include paths, that's fine with me. I just want to make sure that we cover both the "approve limited paths" and "view which paths need to be approved" use cases :)

Tim Hockin

unread,
Sep 26, 2018, 5:21:22 PM9/26/18
to Tim Pepper, Brendan Burns, Davanum Srinivas, Jordan Liggitt, Benjamin Elder, jbe...@redhat.com, Quinton Hoole, Clayton Coleman, Hemant Kumar, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
On Wed, Sep 26, 2018 at 1:16 PM Timothy Pepper <tpe...@vmware.com> wrote:

I’m interested to know more how often top level folks want to “approve limited paths” and if there’s a pattern there which argues for OWNERS files updates to make delegation formal.  For deps, should vendor/OWNERS have some filters applied instead of just:

approvers:

- dep-approvers


I don't understand what the filters would do?  As a top-level approver I have the right to approve PRs in whole, but very frequently I want to approve them only partially.  Since I can't do that, I am forced to wait until the rest of the PR is LGTM and them be the last approver.
 

The “view which paths need to be approved” case is covered today.   Not sure how to do this durably aside from screen shotting…


Yep, this looks great to me.  We actually have the logic now.  What we don't have is the ability for an over-powered approver to "drop some privs".

Timothy Pepper

unread,
Sep 26, 2018, 5:22:41 PM9/26/18
to Tim Hockin, Brendan Burns, Davanum Srinivas, Jordan Liggitt, Benjamin Elder, jbe...@redhat.com, Quinton Hoole, Clayton Coleman, Hemant Kumar, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion

In your case it may not do anything.  But if you wanted to not be the top level approver on the whole lot of it, you could delegate subparts to people.

 

-- 

Tim Pepper

Orchestration & Containers Lead

VMware Open Source Technology Center

 

From: Tim Hockin <tho...@google.com>


Date: Wednesday, September 26, 2018 at 2:21 PM
To: Timothy Pepper <tpe...@vmware.com>

Cc: Brendan Burns <bbu...@microsoft.com>, Davanum Srinivas <dav...@gmail.com>, Jordan Liggitt <jor...@liggitt.net>, Benjamin Elder <benth...@google.com>, "jbe...@redhat.com" <jbe...@redhat.com>, Quinton Hoole <qui...@hoole.biz>, Clayton Coleman <ccol...@redhat.com>, Hemant Kumar <hek...@redhat.com>, "kubernetes-s...@googlegroups.com" <kubernetes-s...@googlegroups.com>, Kubernetes developer/contributor discussion <kuberne...@googlegroups.com>
Subject: Re: Decouple /lgtm and /approve (important especially for top level approvers)

 

On Wed, Sep 26, 2018 at 1:16 PM Timothy Pepper <tpe...@vmware.com> wrote:

Benjamin Elder

unread,
Sep 26, 2018, 6:09:16 PM9/26/18
to Tim Pepper, Tim Hockin, bbu...@microsoft.com, Davanum Srinivas, jor...@liggitt.net, jbe...@redhat.com, qui...@hoole.biz, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, kubernetes-dev, Cole Wagner
+Cole who works on the OWNERS tooling

Dropping privs sounds really interesting, could you file a test-infra issue with what that should look like?

--
You received this message because you are subscribed to the Google Groups "kubernetes-sig-contribex" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-con...@googlegroups.com.
To post to this group, send email to kubernetes-s...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-contribex/7C80E4C5-6E92-4C09-BC8E-382D5F191E30%40vmware.com.

Erick Fejta

unread,
Sep 26, 2018, 8:03:44 PM9/26/18
to Benjamin Elder, Tim Pepper, Tim Hockin, bbu...@microsoft.com, Davanum Srinivas, jor...@liggitt.net, jbe...@redhat.com, qui...@hoole.biz, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, kubernetes-dev, Cole Wagner
We do nothing with the github reveiw status (although the code exists). Can we start?
  • Leaving an approving review is equivalent to both /lgtm and /approve
    • Leaving a request changes review is equivalent to /lgtm cancel and /approve cancel
  • Leaving /lgtm only does /lgtm
  • Leaving /approve only does /approve.

I think this does a good job balancing diverse needs -- people who don't need/care about the precision can use github's built in approval system. Other people can use the explicit, fine-grained commands.

How would we feel about replacing the /hold command with a /merge command (aka replacing the current exclude from merge label with an include in merge one?) This would allow another level of protection, and we could restrict it appropriately.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-contribex/CAOZRXm9-oOi2EGC0s90DCX4X%2BbVYOU6CPb5s9-2AwW_a9WpdLw%40mail.gmail.com.

Brian Grant

unread,
Sep 26, 2018, 8:24:01 PM9/26/18
to Davanum Srinivas, kubernetes-s...@googlegroups.com, Kubernetes-dev

On Wed, Sep 26, 2018 at 7:26 AM Davanum Srinivas <dav...@gmail.com> wrote:
Team,

Please take a look at the change proposed in:

Currently folks who are in the top level OWNERS files have an issue when the use /lgtm. As currently configured the bots assume that the /lgtm is the same as /approve and end up merging PRs. Sometimes, the OWNERS want to just say (for example) that this looks good, but go ask others for approvals. So let's explicitly require them to specify their approval when they wish and not confuse it with a LGTM.

Does this look good? Please chime in the PR above. 

Let's use lazy consensus at least a week (may be merge Oct 5 if there are no issues).

Thanks,
Dims

--
Davanum Srinivas :: https://twitter.com/dims

--
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/CANw6fcF242-LgCXJmrgjzpf-wUWOFo8E%3DSxDpuMF%2BWu4-Gc4Dw%40mail.gmail.com.

Daniel Smith

unread,
Sep 26, 2018, 8:54:47 PM9/26/18
to Erick Fejta, Benjamin Elder, Timothy Pepper, Tim Hockin, bbu...@microsoft.com, Davanum Srinivas, jor...@liggitt.net, jbe...@redhat.com, qui...@hoole.biz, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, kubernetes-dev, Cole Wagner


On Wed, Sep 26, 2018, 5:03 PM 'Erick Fejta' via kubernetes-sig-contribex <kubernetes-s...@googlegroups.com> wrote:
We do nothing with the github reveiw status (although the code exists). Can we start?
  • Leaving an approving review is equivalent to both /lgtm and /approve
    • Leaving a request changes review is equivalent to /lgtm cancel and /approve cancel
The general idea sounds fine but I think approve should be a latch that never gets removed.

Lubomir I. Ivanov

unread,
Sep 26, 2018, 10:36:55 PM9/26/18
to Daniel Smith, Erick Fejta, Benjamin Elder, Timothy Pepper, Tim Hockin, bbu...@microsoft.com, Davanum Srinivas, jor...@liggitt.net, jbe...@redhat.com, qui...@hoole.biz, ccol...@redhat.com, hek...@redhat.com, kubernetes-sig-contribex, kubernetes-dev, Cole Wagner
On 27 September 2018 at 03:54, 'Daniel Smith' via
kubernetes-sig-contribex <kubernetes-s...@googlegroups.com>
wrote:
>
>
> On Wed, Sep 26, 2018, 5:03 PM 'Erick Fejta' via kubernetes-sig-contribex
> <kubernetes-s...@googlegroups.com> wrote:
>>
>> We do nothing with the github reveiw status (although the code exists).
>> Can we start?
>>
>> Leaving an approving review is equivalent to both /lgtm and /approve
>>
>> Leaving a request changes review is equivalent to /lgtm cancel and
>> /approve cancel
>
> The general idea sounds fine but I think approve should be a latch that
> never gets removed.
>>

giving the approver the flex to cancel is a good thing though. e.g.
someone sends good code at first, the PR gets approved, later the code
is changed to something badt and now anyone can LGTM it. the approver
cannot cancel.
> https://groups.google.com/d/msgid/kubernetes-sig-contribex/CAB_J3bar0wsJKp7LGBxBQSXkUVO-xrEZvMxRxXGmam9%2BusKjdw%40mail.gmail.com.

Tim Hockin

unread,
Sep 26, 2018, 11:51:33 PM9/26/18
to Lubomir I. Ivanov, Daniel Smith, Erick Fejta, Benjamin Elder, Timothy Pepper, bbu...@microsoft.com, Davanum Srinivas, jor...@liggitt.net, jbe...@redhat.com, qui...@hoole.biz, ccol...@redhat.com, hek...@redhat.com, kubernetes-sig-contribex, kubernetes-dev, Cole Wagner
Agree, I actually think a new code push should probably nix an approval...

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/CAGDbWi_YTAP3woukp4XPp5VPyBCA9-dngfgUf4i7N2fe%3D900Yg%40mail.gmail.com.

Erick Fejta

unread,
Sep 27, 2018, 12:06:21 AM9/27/18
to Tim Hockin, Lubomir I. Ivanov, Daniel Smith, Benjamin Elder, Timothy Pepper, bbu...@microsoft.com, Davanum Srinivas, jor...@liggitt.net, jbe...@redhat.com, qui...@hoole.biz, ccol...@redhat.com, hek...@redhat.com, kubernetes-sig-contribex, kubernetes-dev, Cole Wagner
/approve cancel is already a command (also /lgtm cancel). Are these unknown or do they work incorrectly?

Brian Grant

unread,
Sep 27, 2018, 1:54:10 AM9/27/18
to Erick Fejta, Tim Hockin, neol...@gmail.com, Daniel Smith, Benjamin Elder, Tim Pepper, Brendan Burns, Davanum Srinivas, Jordan Liggitt, Josh Berkus, Quinton Hoole, Clayton Coleman, Hemant Kumar, kubernetes-s...@googlegroups.com, Kubernetes-dev, Cole Wagner
I, at least, had missed that. Thanks.

Since new commands get added occasionally and we get new contributors all the time, a regular PSA about the available commands, highlighting new, heavily used, and possibly little-known commands might be useful.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-contribex/CAMMDcuGY6CmQ2NsBEz-6Ax3PTf5JFbb%2ByH0TikQXthwyYxks%3DA%40mail.gmail.com.

Kris Nova

unread,
Sep 27, 2018, 3:00:33 AM9/27/18
to Brian Grant, fe...@google.com, tho...@google.com, neol...@gmail.com, dbs...@google.com, benth...@google.com, tpe...@vmware.com, bbu...@microsoft.com, dav...@gmail.com, jor...@liggitt.net, jbe...@redhat.com, qui...@hoole.biz, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, kubernetes-dev, co...@google.com
Can't agree with Brian enough.

The commands are confusing and frustrating enough as it is (especially for new contributors). If we are changing the paradigms behind the commands - we should at least do our best to share these changes broadly and regularly (even if that means repeating ourselves). 





To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/CAKCBhs6vu3V9s%2BKRvAnT4dR%3DvAp%3D_XeHei7x%3D7JdrrkFWoUA5A%40mail.gmail.com.

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


--
Nova

Davanum Srinivas

unread,
Sep 27, 2018, 6:32:09 AM9/27/18
to kn...@heptio.com, brian...@google.com, fe...@google.com, tho...@google.com, neol...@gmail.com, dbs...@google.com, benth...@google.com, tpe...@vmware.com, bbu...@microsoft.com, jor...@liggitt.net, jbe...@redhat.com, qui...@hoole.biz, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion, co...@google.com
In that spirit, here's where the bot commands are documented:

And here's the data for the most used commands:

I'll ping contribex folks to see where we can highlight these for new contributors (if not done already)

-- Dims

Brendan Burns

unread,
Sep 27, 2018, 12:05:42 PM9/27/18
to kn...@heptio.com, Davanum Srinivas, brian...@google.com, fe...@google.com, tho...@google.com, neol...@gmail.com, dbs...@google.com, benth...@google.com, tpe...@vmware.com, jor...@liggitt.net, jbe...@redhat.com, qui...@hoole.biz, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion, co...@google.com
The trouble with a push removing /approve is that a rebase can cause approve removal and it's this really easy to get into rebase hell.

Delegating permissions could make that much better though.

So I think if we're going to make a new push remove approve we need to also add approval delegation.

--brendan

Matthias Bertschy

unread,
Sep 27, 2018, 12:25:58 PM9/27/18
to Brendan Burns, kn...@heptio.com, Davanum Srinivas, brian...@google.com, fe...@google.com, tho...@google.com, neol...@gmail.com, dbs...@google.com, benth...@google.com, tpe...@vmware.com, jor...@liggitt.net, jbe...@redhat.com, qui...@hoole.biz, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion, co...@google.com
On this particular point (for lgtm) I have a PR that should be finished soon:

Maybe the same hash could be checked for approve...

Best regards,
Matthias

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-dev/CY4PR21MB0504E411404B52A516023C6ADB140%40CY4PR21MB0504.namprd21.prod.outlook.com.

Stephen Augustus

unread,
Sep 27, 2018, 12:32:11 PM9/27/18
to Brendan Burns, kn...@heptio.com, Davanum Srinivas, brian...@google.com, fe...@google.com, tho...@google.com, neol...@gmail.com, dbs...@google.com, benth...@google.com, tpe...@vmware.com, jor...@liggitt.net, jbe...@redhat.com, qui...@hoole.biz, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion, co...@google.com
Hmmm, I thought new pushes killed lgtms, but not approves.

-- Stephen

Brendan Burns

unread,
Sep 27, 2018, 1:18:19 PM9/27/18
to Stephen Augustus, kn...@heptio.com, Davanum Srinivas, brian...@google.com, fe...@google.com, tho...@google.com, neol...@gmail.com, dbs...@google.com, benth...@google.com, tpe...@vmware.com, jor...@liggitt.net, jbe...@redhat.com, qui...@hoole.biz, ccol...@redhat.com, hek...@redhat.com, kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion, co...@google.com
That's the current state, but several people on this thread were advocating to change it to cover approve too.

--brendan

Tim Hockin

unread,
Sep 27, 2018, 1:55:05 PM9/27/18
to Brendan Burns, kn...@heptio.com, Davanum Srinivas, Brian Grant, Erick Fejta, Lubomir I. Ivanov, Daniel Smith, Benjamin Elder, Tim Pepper, Jordan Liggitt, jbe...@redhat.com, Quinton Hoole, Clayton Coleman, Hemant Kumar, kubernetes-sig-contribex, Kubernetes developer/contributor discussion, Cole Wagner
...or make lgtm/approval per per-file-checksum and only remove it if
those particular files changed.

Brendan Burns

unread,
Sep 27, 2018, 2:06:04 PM9/27/18
to Tim Hockin, kn...@heptio.com, Davanum Srinivas, Brian Grant, Erick Fejta, Lubomir I. Ivanov, Daniel Smith, Benjamin Elder, Tim Pepper, Jordan Liggitt, jbe...@redhat.com, Quinton Hoole, Clayton Coleman, Hemant Kumar, kubernetes-sig-contribex, Kubernetes developer/contributor discussion, Cole Wagner
It would be really awesome actually if we could detect and automate rebases, and have the automation preserve /approve and /lgtm, but now I'm just dreaming :)


From: Tim Hockin <tho...@google.com>
Sent: Thursday, September 27, 2018 10:54 AM
To: Brendan Burns
Cc: kn...@heptio.com; Davanum Srinivas; Brian Grant; Erick Fejta; Lubomir I. Ivanov; Daniel Smith; Benjamin Elder; Tim Pepper; Jordan Liggitt; jbe...@redhat.com; Quinton Hoole; Clayton Coleman; Hemant Kumar; kubernetes-sig-contribex; Kubernetes developer/contributor discussion; Cole Wagner

Subject: Re: Decouple /lgtm and /approve (important especially for top level approvers)
...or make lgtm/approval per per-file-checksum and only remove it if
those particular files changed.
On Thu, Sep 27, 2018 at 9:05 AM Brendan Burns <bbu...@microsoft.com> wrote:
>
> The trouble with a push removing /approve is that a rebase can cause approve removal and it's this really easy to get into rebase hell.
>
> Delegating permissions could make that much better though.
>
> So I think if we're going to make a new push remove approve we need to also add approval delegation.
>
> --brendan
>
> ________________________________
> From: Davanum Srinivas <dav...@gmail.com>
> Sent: Thursday, September 27, 2018 3:31:53 AM
> To: kn...@heptio.com
> Cc: brian...@google.com; fe...@google.com; tho...@google.com; neol...@gmail.com; dbs...@google.com; benth...@google.com; tpe...@vmware.com; Brendan Burns; jor...@liggitt.net; jbe...@redhat.com; qui...@hoole.biz; ccol...@redhat.com; hek...@redhat.com; kubernetes-s...@googlegroups.com; Kubernetes developer/contributor discussion; co...@google.com
> Subject: Re: Decouple /lgtm and /approve (important especially for top level approvers)
>
> In that spirit, here's where the bot commands are documented:

>
> And here's the data for the most used commands:

>>>>>> >>>
>>>>>> >>> --
>>>>>> >>> You received this message because you are subscribed to the Google Groups
>>>>>> >>> "kubernetes-sig-contribex" group.
>>>>>> >>> To unsubscribe from this group and stop receiving emails from it, send an
>>>>>> >>> email to kubernetes-sig-con...@googlegroups.com.
>>>>>> >>> To post to this group, send email to
>>>>>> >>> kubernetes-s...@googlegroups.com.
>>>>>> >>> To view this discussion on the web visit

>>>>>> >>
>>>>>> >> --
>>>>>> >> You received this message because you are subscribed to the Google Groups
>>>>>> >> "kubernetes-sig-contribex" group.
>>>>>> >> To unsubscribe from this group and stop receiving emails from it, send an
>>>>>> >> email to kubernetes-sig-con...@googlegroups.com.
>>>>>> >> To post to this group, send email to
>>>>>> >> kubernetes-s...@googlegroups.com.
>>>>>> >> To view this discussion on the web visit

>>>>>> >
>>>>>> > --
>>>>>> > You received this message because you are subscribed to the Google Groups
>>>>>> > "kubernetes-sig-contribex" group.
>>>>>> > To unsubscribe from this group and stop receiving emails from it, send an
>>>>>> > email to kubernetes-sig-con...@googlegroups.com.
>>>>>> > To post to this group, send email to
>>>>>> > kubernetes-s...@googlegroups.com.
>>>>>> > To view this discussion on the web visit

>>>>>>
>>>>>> --
>>>>>>
>>>>>> 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://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fkubernetes-dev%2FCAGDbWi_YTAP3woukp4XPp5VPyBCA9-dngfgUf4i7N2fe%253D900Yg%2540mail.gmail.com&amp;data=02%7C01%7Cbburns%40microsoft.com%7Cf55130e478c740e5edaa08d624a25979%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636736677037301985&amp;sdata=bIcuyKd1kBIueHjMPpCVyDlGRi4CrfmU5iJL4UGCUzM%3D&amp;reserved=0.
>>>>>>
>>>>>>
>>>>>> For more options, visit https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Foptout&amp;data=02%7C01%7Cbburns%40microsoft.com%7Cf55130e478c740e5edaa08d624a25979%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636736677037301985&amp;sdata=uOOvV34NloJ%2Bh1mlr2vDdfHDR7tztQ0TMOiJwxkwt3Y%3D&amp;reserved=0.

>>>>
>>>> --
>>>> You received this message because you are subscribed to the Google Groups "kubernetes-sig-contribex" group.
>>>> To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-con...@googlegroups.com.
>>>> To post to this group, send email to kubernetes-s...@googlegroups.com.
>>>> To view this discussion on the web visit https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fkubernetes-sig-contribex%2FCAMMDcuGY6CmQ2NsBEz-6Ax3PTf5JFbb%252ByH0TikQXthwyYxks%253DA%2540mail.gmail.com&amp;data=02%7C01%7Cbburns%40microsoft.com%7Cf55130e478c740e5edaa08d624a25979%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636736677037301985&amp;sdata=qLUpIdgpwP5YRtRFzoSZPNmYeks6gcylHLRv0GrsO7U%3D&amp;reserved=0.
>>>> For more options, visit https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Foptout&amp;data=02%7C01%7Cbburns%40microsoft.com%7Cf55130e478c740e5edaa08d624a25979%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636736677037301985&amp;sdata=uOOvV34NloJ%2Bh1mlr2vDdfHDR7tztQ0TMOiJwxkwt3Y%3D&amp;reserved=0.

>>>
>>> --
>>> 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://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fkubernetes-dev%2FCAKCBhs6vu3V9s%252BKRvAnT4dR%253DvAp%253D_XeHei7x%253D7JdrrkFWoUA5A%2540mail.gmail.com&amp;data=02%7C01%7Cbburns%40microsoft.com%7Cf55130e478c740e5edaa08d624a25979%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636736677037301985&amp;sdata=bF%2BNFPglRkkXKhaJNHSaCP1izqXvyTue2gNQvPJbzCE%3D&amp;reserved=0.
>>> For more options, visit https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Foptout&amp;data=02%7C01%7Cbburns%40microsoft.com%7Cf55130e478c740e5edaa08d624a25979%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636736677037301985&amp;sdata=uOOvV34NloJ%2Bh1mlr2vDdfHDR7tztQ0TMOiJwxkwt3Y%3D&amp;reserved=0.
>>
>>
>>
>> --
>> Nova
>>
>
>
> --
> Davanum Srinivas :: https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftwitter.com%2Fdims&amp;data=02%7C01%7Cbburns%40microsoft.com%7Cf55130e478c740e5edaa08d624a25979%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636736677037311993&amp;sdata=4Pb2N6Zo2Cb%2BmzO%2FMgWOaQ8aoUz4yZ2hZdjowkEmd0M%3D&amp;reserved=0

Antoine Pelisse

unread,
Sep 27, 2018, 2:08:28 PM9/27/18
to bbu...@microsoft.com, Tim Hockin, kn...@heptio.com, Davanum Srinivas, Brian Grant, Erick Fejta, neol...@gmail.com, Daniel Smith, Benjamin Elder, Tim Pepper, jor...@liggitt.net, Josh Berkus, qui...@hoole.biz, Clayton Coleman, hek...@redhat.com, kubernetes-s...@googlegroups.com, kubernetes-dev, Cole Wagner
On Thu, Sep 27, 2018 at 11:06 AM 'Brendan Burns' via Kubernetes developer/contributor discussion <kuberne...@googlegroups.com> wrote:
It would be really awesome actually if we could detect and automate rebases, and have the automation preserve /approve and /lgtm, but now I'm just dreaming :)

rebases are mostly required because of conflicts, and I'm not sure we want a machine to fix conflicts. Making rebases easier by improving the tooling sounds great though. 

Andrew Kutz

unread,
Sep 27, 2018, 7:59:16 PM9/27/18
to Aaron Crickenberger, Kubernetes developer/contributor discussion
/lgtm

--
-a

Andrew Kutz
Engineer, VMware Cloud-Native Apps BU
ak...@vmware.com
512-658-8368


On 9/26/18, 12:32 PM, "kuberne...@googlegroups.com on behalf of Aaron Crickenberger" <kuberne...@googlegroups.com on behalf of spi...@gmail.com> wrote:

I cannot +1 Dims' proposal enthusiastically enough


- aaron

On Wednesday, September 26, 2018 at 10:18:19 AM UTC-7, Josh Berkus wrote:

On 09/26/2018 09:19 AM, Quinton Hoole wrote:
> I agree with Tim, and the main proposal. In my mind /lgtm implies that
> I have reviewed the whole PR. /approve implies that I have verified
> that the right set of people have reviewed the PR adequately, and in my
> judgement it's good to go (even if I have not reviewed it myself).
>

The problem is that what /approve means and what /lgtm means depends on
which repository/SIG you're dealing with.

For some teams:
/lgtm is equivalent to +1
/approve is "merge it"

For other teams:
/approve is "meets standards, OK to merge if LGTM'd"
/lgtm is "merge it"

This makes it difficult to maintain community standards for how the two
labels are to be used, or to train new contributors. We don't need to
change labels as much as we need to make practices consistent. If that
involves changing labels, I'm for it, but changing labels alone won't
fix anything.

--
--
Josh Berkus
Kubernetes Community
Red Hat OSAS




--
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 <mailto:kuberne...@googlegroups.com>.
To view this discussion on the web visit
https://groups.google.com/d/msgid/kubernetes-dev/07c63405-ef3e-4c47-a493-61c2894cca99%40googlegroups.com <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fkubernetes-dev%2F07c63405-ef3e-4c47-a493-61c2894cca99%2540googlegroups.com%3Futm_medium%3Demail%26utm_source%3Dfooter&data=02%7C01%7Cakutz%40vmware.com%7Ce3ce9d49a41e42d1213f08d623d5f6bb%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636735799223791929&sdata=1KKhwLPP%2FHk9LrbcWpw7qS4G9y9V%2F%2Fp8%2FxB07IYLOCI%3D&reserved=0>.
For more options, visit
https://groups.google.com/d/optout <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Foptout&data=02%7C01%7Cakutz%40vmware.com%7Ce3ce9d49a41e42d1213f08d623d5f6bb%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636735799223791929&sdata=%2FhlcQN32rl%2BpSMWtaMyTtKZagHNkVs%2BJ4R0gOGScDhM%3D&reserved=0>.


Paris Pittman

unread,
Sep 28, 2018, 3:05:08 PM9/28/18
to Andrew Kutz, Aaron Crickenberger, Kubernetes developer/contributor discussion
plug: the contributor experience survey is still live! We have 160 responses; 200 would make for a great sample size. Automation request fields are in there!


:) happy friday




    To post to this group, send email to
    kubernetes-dev@googlegroups.com <mailto:kubernetes-dev@googlegroups.com>.

    To view this discussion on the web visit
    https://groups.google.com/d/msgid/kubernetes-dev/07c63405-ef3e-4c47-a493-61c2894cca99%40googlegroups.com <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fkubernetes-dev%2F07c63405-ef3e-4c47-a493-61c2894cca99%2540googlegroups.com%3Futm_medium%3Demail%26utm_source%3Dfooter&data=02%7C01%7Cakutz%40vmware.com%7Ce3ce9d49a41e42d1213f08d623d5f6bb%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636735799223791929&sdata=1KKhwLPP%2FHk9LrbcWpw7qS4G9y9V%2F%2Fp8%2FxB07IYLOCI%3D&reserved=0>.
    For more options, visit
    https://groups.google.com/d/optout <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Foptout&data=02%7C01%7Cakutz%40vmware.com%7Ce3ce9d49a41e42d1213f08d623d5f6bb%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636735799223791929&sdata=%2FhlcQN32rl%2BpSMWtaMyTtKZagHNkVs%2BJ4R0gOGScDhM%3D&reserved=0>.
--
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/D2F35EE0-E9E4-4684-85B9-AEF26ACBA44A%40vmware.com.
For more options, visit https://groups.google.com/d/optout.



--

Paris Pittman

Kubernetes Community

Open Source Strategy, Google Cloud

345 Spear Street, San Francisco, 94105


Davanum Srinivas

unread,
Oct 8, 2018, 9:40:58 PM10/8/18
to kubernetes-s...@googlegroups.com, Kubernetes developer/contributor discussion
https://github.com/kubernetes/test-infra/pull/9151 is going in now.

Any volunteers who can scan the rest of the messages from this thread and propose follow ups / work items?

Thanks,
Dims

On Wed, Sep 26, 2018 at 10:26 AM Davanum Srinivas <dav...@gmail.com> wrote:
Team,

Please take a look at the change proposed in:

Text from the PR:

Currently folks who are in the top level OWNERS files have an issue when the use /lgtm. As currently configured the bots assume that the /lgtm is the same as /approve and end up merging PRs. Sometimes, the OWNERS want to just say (for example) that this looks good, but go ask others for approvals. So let's explicitly require them to specify their approval when they wish and not confuse it with a LGTM.

Does this look good? Please chime in the PR above. 

Let's use lazy consensus at least a week (may be merge Oct 5 if there are no issues).

Thanks,
Dims

--
Davanum Srinivas :: https://twitter.com/dims
Reply all
Reply to author
Forward
0 new messages