Test-infra: Allow /lgtm to not also mean /approve

[spiffxp]

I would like to see an option for the approve plugin that allows /lgtm to only mean /lgtm, instead of /lgtm+/approve. Something like lgtmActsAsApprove with a default of false

It's more explicit and less magical to a newcomer. It would make accurate reporting on approvers less complicated. There are times where I want to act just as a reviewer instead of an approver, and I can't, because I'm in an OWNERS file somewhere up in the hierarchy.

I suppose the other option would be to add an /lgtm-only but really, making these commands explicit seems the clearer path forward.

/area prow

@kubernetes/sig-testing-feature-requests @kubernetes/sig-contributor-experience-feature-requests WDYT?

[Bradamant3]

This would help the maintainers of k8s/website a lot. We've just adopted the merge bot, and having approvers up the chain able to merge without realizing it makes both tech review and doc review harder. For sig-docs, we're clear about the distinction. But we can't expect everyone to remember what happens in downchain repos ...

[cblecker]

I support this. Most approvers know the command, and do /lgtm + /approve anyways.

[mattfarina]

I would ask that this be configurable. There are some places, such as charts, that would want them the same.

Most approvers know the command, and do /lgtm + /approve anyways.

When I looked at devstats developers summary, that Aaron showed off in community today, I noticed that multiple people in the top 10 for review comments are on projects that use /lgtm to also mean /approve.

I support the configurable option.

[cblecker]

To clarify: When I said "most approvers" I mean "most approvers that I personally see, anecdotally". :)

I also support a configurable option.

[liggitt]

I'm curious under what circumstances someone would have reviewed and LGTMed and would not approve of the change. LGTM is supposed to represent a more detailed review, right?

[fejta]

I don't think this should be the default, as who lgtms without approving?

I would be a lot happier with an /lgtm-only command that people can use.

I don't think this should be a flag, as it will be confusing to have /lgtm do different things in differ repos.

So ideally I would like three commands: one for only lgtm, one for only approve and one for both.

If someone wants to /lgtm without causing a merge I'd recommend training people to use /lgtm /hold.

[Bradamant3]

@liggitt take a look at https://github.com/kubernetes/website/pull/7164. Michelle meant simply to say that she signed off on the review. But bc she's an approver up the chain, the PR was merged.

'Also, for docs, we thought to keep the distinction between the old Tech Review LGTM and Docs Review LGTM labels, with /lgtm signifying tech review done and /approve signifying docs review done. I don't think /lgtm /hold solves this problem.

There was a /hold on the PR I just cited, but it needed to be there for technical content reasons. Once those fixes were in place, how else would we signify that the PR was ready for further processing?

[stevekuznetsov]

The intent behind the two today is:

• /lgtm means "this code has been implemented well"
• /approve means "this code is making a useful change and is in line with the goals of the project"

I think the crux behind the kubernetes/website issue is that the repo uses an entirely different approach for their reviews and, while it's possible to map Tech LGTM and Docs LGTM to the two concepts above, or use the /hold and /hold cancel to approximate the desired behavior in the website repo, either option is still missing the mark of how the repo contributors expect their review flows to work.

From a testing infrastructure perspective, it's easy to suggest a mapping of roles or the use of /hold to remedy this as it requires no more infrastructure work, but I wonder if there is any way to support the Tech LGTM and Docs LGTM concepts (and others as they come up) without ballooning the test-infra codebase to implement each workflow. Similarly, how does this fit into the larger steering committee goals about common workflows across the k8s ecosystem repos?

[Bradamant3]

@stevekuznetsov The definitions of intent are super helpful. If we're really going to align contributors' expectations across repos, I suspect we have even more work to do. I'm taking this issue to sig-docs for more discussion, too.

But a definition of intent is also something a bit different from bot behavior. It's the understanding of sig-docs that the behavior once a label is applied depends on the maintainer's role in the OWNERS file (reviewer vs approver). So there's a matrix of meanings, instead of the labels always and only meaning one thing regardless of who applies them. Maybe we need to pull apart the two things (label intent vs bot behavior), for purposes of discussion at least?

[cblecker]

The other key piece from Aaron's initial comment:

It would make accurate reporting on approvers less complicated.

If we make intent clear, we can better report in it.

[liggitt]

/lgtm means "this code has been implemented well"

I'm not sure its scope is that narrow. A situation where a lgtm by an owner doesn't mean they approve is really confusing

[fejta]

I do not see a use case for lgtm without approve, except as a synonym for "I like this change but don't want it to merge yet for whatever reason." People should use /hold and /hold cancel for this, not depending on approve/lgtm specifics.

Expectations should be that /lgtm and /approve generally happen at the same time: people reviewing code should also be the owners of that code. Exceptions should be unusual.

The intended use case for using /approve without /lgtm is a sudden, overwhelming influx of PRs to a particular area -- more than the existing set of owners can quickly review. This a great time for an expert in the area to /approve the general concept and delegate careful review of the specifics to someone more junior.

However a sustained, overwhelming volume of PRs is a just signal that the area needs more help. The priority should be to increase expertise in the area and expand the number of owners who can do reviews.

PS: Bradamant3, I do not see anyone using /hold in the website PR you linked. Holds typically look more like the flow in this PR: https://github.com/kubernetes/test-infra/pull/6258#issuecomment-362117453

[liggitt]

I do not see a use case for lgtm without approve, except as a synonym for "I like this change but don't want it to merge yet for whatever reason." People should use /hold and /hold cancel for this, not depending on approve/lgtm specifics.

+1

[heckj]

I would prefer to have the two concepts separate - it's surprisingly unclear to have lgtm represent dual meanings based on who you are. The SIG-DOCS group explicitly wants to leverage lgtm and approve separately to make sure we have both an editorial and technical review for the content going into kubernetes/website.

Right now getting this functionality implies a mandatory "opt out" that only approvers need to apply (i.e. /lgtm + /hold) for the merging, when I think it would be better to 'opt in' - for example, using /lgtm + /approve when desired)

[liggitt]

I think it would be better to 'opt in'

I think the majority of uses fall the other way... it's rare for someone with approve rights to do a detailed review, say it looks good to them, and not want it to be approved

[liggitt]

I actually think it's confusing for other people to have a PR with LGTM by an owner that isn't merging without some /hold comment explaining why. "This looked good to alice, and she's an owner... what could the hold up be?"

[steveperry-53]

To those of you who have been saying that you don't see a use case for lgtm without approve: The use case is in kubernetes/website. We need to distinguish between tech review and writing review. We want the writing reviewer to have final approval, but we also want an lgtm from a tech reviewer. I'd be fine with with having true as the default value of lgtmActsAsApprove. But for kubernetes/website, I really want to have the option of setting lgtmActsAsApprove to false.

[fejta]

I still argue that has nothing to do with lgtm vs approve. You are trying to convert approval into something else that meets the desires of your repo, which would be more correctly expressed as something like writer-lgtm + tech-lgtm and writer-approve + tech-approve.

Approval is about assigning owners to each file and ensuring that every change to a file is approved by an owner of that file. It is neither a tech review nor a writing review.

[Bradamant3]

It's not just the desires of a single repo. The issue came up because someone NOT a k8s/website maintainer issued an /lgtm she did NOT expect to trigger a merge. But bc she's in the OWNERS file as an approver for k8s/k8s, her /lgtm did trigger a merge.

[liggitt]

bc she's in the OWNERS file as an approver for k8s/k8s, her /lgtm did trigger a merge.

OWNERS files are per folder, per repo.

[mattfarina]

Reading this thread a few things come to mind:

• We can't make all the people happy all of the time.
• We have different tailored processes on some different repos. This is ok (even good) because it solves the nuances of the problems we have. Not too much process but just enough for the situation at hand.
• Many people may not know how /lgtm, /approve, and the automation work. I've run into this several times this month.

The last bullet is a contribX problem worth trying to solve. It's above and beyond what happens here.

cc @parispittman ^^

[chenopis]

We're talking about two different use cases here, so let's not get lost in generalities. In docs, we need both docs and tech reviews, so for us, we need /lgtm to be separate from /approve so that the PR is not merged before both reviews are given.

If this behavior can be configured per repo, I think we'll all be happy. I don't want to dictate the behavior for other repos if they have different use cases.

[fejta]

we need both docs and tech reviews, so for us, we need /lgtm to be separate from /approve

The first half of this sentence does not imply the second

And eventually/ideally main repos all provide the same, consistent experience to make it easy to move from one to the next

[chenopis]

@fejta For kubernetes/website, we operate differently in that we (sig-docs-maintainers) give the final say so on if/when a PR should be merged, regardless of who "owns" the files. i.e. We are the approvers.

[BenTheElder]

Do you not effectively own the files then? This sounds exactly like what
approver is supposed to do.

On Wed, Feb 7, 2018 at 2:27 PM, Andrew Chen [e-mail]
wrote:

@fejta https://github.com/fejta For kubernetes/website, we operate
differently in that we (sig-docs-maintainers) give the final say so on
if/when a PR should be merged, regardless of who "owns" the files. i.e. We
are the approvers.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/6589#issuecomment-363932606,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA4Bq9YrEYjk6LGy7poodpEi1BzoHDxXks5tSiNagaJpZM4R2PlA
.

[chenopis]

I'm not sure I understand why there's pushback on this. Can't we just have a configurable option, as @spiffxp is suggesting, and call it a day? The code contribution and docs contribution workflows are different, so I see no reason to force one on the other.

[fejta]

We should not provide a configurable option because we want /lgtm and /approve to operate the same way in any repo in the org.

sig-docs-maintainers) give the final say

Then stop giving devs approval rights in OWNERS files? An /lgtm or /approve will only approve changes to files that the commenter is an approver for. We only add the approve label when all files have been approved.

regardless of who "owns" the files

If you do not care who owns the files then this has nothing to do with the behavior of the /approve command and the approval plugin.

You want PRs to get both a doc review and a dev review? Great. Write a doc-review plugin that defines and enforces this workflow.

Want to try and force a square doc-review workflow through a round approval plugin anyway? Okay... But any resulting awkwardness and undesirable behavior is not the fault of the approval plugin, which is trying to do something else.

[parispittman]

@fejta - can you bring this to the community meeting tomorrow? possibly the announcements section? I'm hearing from lots of folks that they are confused and I don't think folks are on the same page.

[jaredbhatti]

I have to miss the Contrib-x meeting due to a meeting conflict, but I agree with Andrew. I'd prefer a configurable process. The SIG-Docs team uses two different triggers for content review on the site, one for technical review, one for content review. We specifically pulled these two steps apart to prevent content from going live before the code was live, and to prevent poorly formatted content from going out the door before we reviewed. Both of these issues happened in the past and insisting on a two-step review fixed the problem.

[fejta]

@jaredbhatti @chenopis let's move the sig-docs concerns over to https://github.com/kubernetes/website/issues/7289

what you say you want and what you configured are pretty far apart.

[chenopis]

The configuration is still in flux. For some historical reference: our files used to contain assignees, which was deprecated. Consequently, @spiffxp helped us to convert them to use approvers, see https://github.com/kubernetes/website/pull/4607. So let's just stipulate that the repo may not be in its desired final state.

Can we first just agree on what behavior we want and then configure it accordingly?

[bgrant0607]

I think explicit /approve makes sense.

[steveperry-53]

In the docs repo (kubernetes/website) we have solved most of the problem by
editing our OWNERS files. In the OWNERs files, we moved most people out of
the approvers list and into the reviewers list. A PR will not get
automatically merged unless someone in the approvers list gives the
/approve command. So with a smaller list of approvers, we are getting fewer
accidental merges.

For an automatic merge, in addition to an /approve from an approver, we
need an /lgtm. Our thought was to go with the convention that /lgtm means
technical approval and /approve means writing approval.

The remaining problem, as I see it:
The convention (/lgtm = tech, /approve = writing) is a bit of a secret
known only to those of us who have agreed on it. So a potential writing
reviewer who is not in our inner circle won't necessarily know that /lgtm
means tech approval. And a potential tech reviewer who is not in our inner
circle probably won't know that they should use /lgtm instead of /approve.

Possible solution: In kubernetes/website, we might want to have a
/lgtm-tech command.

On Thu, Feb 15, 2018 at 11:06 AM, Brian Grant [e-mail]
wrote:

I think explicit /approve makes sense.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/6589#issuecomment-366029078,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ANJ2DrqJh84MuN0Lm5lDcL0a3TvYh7Vqks5tVIAugaJpZM4R2PlA
.

[spiffxp]

I'm still of the opinion it would be better for newcomers if we made things more explicit, ie:

• 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 followed up on kubernetes-dev@ to try and reach a wider audience, as thus far we've only heard from sig-docs, sig-contribex, and a few k/k approvers

https://groups.google.com/forum/#!topic/kubernetes-dev/PJJxV9roXI8

[duglin]

I wonder if the word 'approve' isn't part of the confusion here. To a newbie the word "approve" and "LGTM" probably imply the same thing. In other projects I've seen them avoid this by using "SGTM" (sounds good to me). Even w/o having read any formal docs on what that means, most people seem to instinctively understand that it doesn't mean that someone reviewed and approved the specific code changes - rather they "like the idea" behind the PR. So, SGTMs would never get a PR merged, only LGTMs would, but SGTMs still provide the "good idea" check that I think people are looking for - but w/o the potential overlapping definition of words (like "approve" might).