New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow /lgtm to not also mean /approve #6589

Closed
spiffxp opened this Issue Feb 1, 2018 · 35 comments

Comments

Projects
None yet
@spiffxp
Member

spiffxp commented Feb 1, 2018

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 comment has been minimized.

Member

Bradamant3 commented Feb 1, 2018

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

This comment has been minimized.

Member

cblecker commented Feb 1, 2018

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

@mattfarina

This comment has been minimized.

Member

mattfarina commented Feb 1, 2018

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

This comment has been minimized.

Member

cblecker commented Feb 1, 2018

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

I also support a configurable option.

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 1, 2018

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

This comment has been minimized.

Contributor

fejta commented Feb 1, 2018

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

This comment has been minimized.

Member

Bradamant3 commented Feb 1, 2018

@liggitt take a look at kubernetes/website#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

This comment has been minimized.

Contributor

stevekuznetsov commented Feb 1, 2018

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

This comment has been minimized.

Member

Bradamant3 commented Feb 1, 2018

@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

This comment has been minimized.

Member

cblecker commented Feb 1, 2018

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

This comment has been minimized.

Member

liggitt commented Feb 2, 2018

/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

This comment has been minimized.

Contributor

fejta commented Feb 2, 2018

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: #6258 (comment)

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 2, 2018

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

This comment has been minimized.

Member

heckj commented Feb 6, 2018

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

This comment has been minimized.

Member

liggitt commented Feb 6, 2018

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

This comment has been minimized.

Member

liggitt commented Feb 6, 2018

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

This comment has been minimized.

Contributor

steveperry-53 commented Feb 6, 2018

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

This comment has been minimized.

Contributor

fejta commented Feb 6, 2018

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

This comment has been minimized.

Member

Bradamant3 commented Feb 6, 2018

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

This comment has been minimized.

Member

liggitt commented Feb 6, 2018

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

This comment has been minimized.

Member

mattfarina commented Feb 7, 2018

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

This comment has been minimized.

Contributor

chenopis commented Feb 7, 2018

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

This comment has been minimized.

Contributor

fejta commented Feb 7, 2018

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

This comment has been minimized.

Contributor

chenopis commented Feb 7, 2018

@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

This comment has been minimized.

Member

BenTheElder commented Feb 7, 2018

@chenopis

This comment has been minimized.

Contributor

chenopis commented Feb 7, 2018

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

This comment has been minimized.

Contributor

fejta commented Feb 7, 2018

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

This comment has been minimized.

parispittman commented Feb 7, 2018

@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

This comment has been minimized.

jaredbhatti commented Feb 8, 2018

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

This comment has been minimized.

Contributor

fejta commented Feb 8, 2018

@jaredbhatti @chenopis let's move the sig-docs concerns over to kubernetes/website#7289

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

@chenopis

This comment has been minimized.

Contributor

chenopis commented Feb 8, 2018

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 kubernetes/website#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

This comment has been minimized.

Member

bgrant0607 commented Feb 15, 2018

I think explicit /approve makes sense.

@steveperry-53

This comment has been minimized.

Contributor

steveperry-53 commented Feb 15, 2018

@spiffxp

This comment has been minimized.

Member

spiffxp commented Feb 16, 2018

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

This comment has been minimized.

Contributor

duglin commented Feb 16, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment