WIP: [mungegithub] OWNERS implementation #474
Conversation
|
Is this ready for review or targetted discussion? can you nominate a reviewer? I'm keen to get this moving, but I am also buried in PRs.. |
|
This code is ready for review. @pmorie for some reason has started looking more and more in this area. But I see this as completely blocked on UI/usability and have no interest in seeing it merged until this owners stuff is discoverable and manageable by both PR authors and owners. And I don't know when I'll get around to playing with UI stuff myself.... |
I think the reason was you started asking me for reviews :) |
| ownerApproval = "has-owner-approval" | ||
| ) | ||
|
|
||
| type assignmentConfig struct { |
There was a problem hiding this comment.
if I interpret correctly, this is the type that the OWNERS file is going to unmarshal into, yes?
Can we make it be a struct so we have room to grow?
type OwnersFile struct {
Owners []string `json:owners`
}
This then leaves me room for adding maintainers or other metadata.
There was a problem hiding this comment.
I'm tired. I just asked you for exactly what you did, modulo JSON tags.
|
This could do with a short doc explaining the semantics. We are sort of lacking in docs for all of the gh bot stuff.. |
|
What is the status of this? |
|
What does this PR do? |
|
This PR adds or removes a label called
Any of the following combinations of people could approve the PR.
We can EASILY make the submit queue require the 'has-owner-approval' label. If it gets added by hand, it will be removed. Now I am unwilling to merge, and I feel like I've written this 10 times, but can't find it, due to UI/UX/usability. We absolutely need 2 things before i think we should turn this on.
Even if I had the time, I'm not sure I have the web design skills to put those 2 UIs together into anything reasonable, but without them, this whole process because quite opaque to developers and almost impossible for owners.... |
|
Thanks, @eparis. The logic about who can approve makes sense to me -- approvers from all touched directories are required. I think LGTM from an OWNER should count as approval. I don't think we need to introduce different text. Ideally, LGTM from OWNERs would apply the LGTM label automatically, as well. With this, I think we could get rid of "ok-to-merge", also: if the required OWNERS say it's good, just merge it already. Maybe the lgtm label could mean LGTM from someone trusted and has-owner-approval could be applied once all required owners have given LGTM. I'd like to drive PR assignment (blunderbuss) from OWENRS, also. The blunderbuss list is currently too small, and many PRs languish with overloaded assignees without being reassigned. Maybe we should even automatically reassign any PR to which the assignee doesn't respond within some amount of time (a day, a week?). The auto-assigner could reassign from a pool of owners that hasn't yet approved in the case that multiple approvers are required, favoring leaf owners, in general. We could maybe provide support to reassign to a trunk approver, using ESCALATE or somesuch. Manual reassignment should work, also, at least until the reassignment timeout. For UX, we could post a message to the PR to describe the workflow and the escalation procedure, and list the leaf and trunk owners. The PR author could notify other owners or escalate if automatic reassignment wasn't working. Distinguishing between partially lgtm and fully approved would be useful for listing PRs in limbo. |
|
I should probably do that separately. I intended that as an enhancement on this, but it is actually reasonable to do first and makes all of the code for this smaller/easier... |
Nothing enforces it, but it should be automatically just adding and removing a label based on 'OWNERS' The kubernetes tree pointed to MUST have an OWNERS file in the root dir or it will absolutely go off the rails.
|
Whats the status on this? Is this PR still alive, or has it been supplanted by others? I see OWNERS files in tree.. |
|
This PR is still alive-ish. I don't know how to write a decent UI for OWNERS. The OWNERS files in the kube tree only currently supports 'assignees'. Which is what the old blunderbuss uses. This PR expands that to also have an owners concept. But I don't want to see it merged until we have a good UI for both owners to know what they need to approve and for PR authors to know where to get approval. bgrant had some ideas (auto-reassign the PR to another owner) but I think that gives a rather 'black box' view for everyone... So the mechanics are here in this PR, but I don't know if I have to time to create a good story for both owners and authors. So I'd love someone else who has time to pick up the ball there... |
|
Dashboard to view PRs that need to be approved, parameterized by approver. |
|
For tests and docs it would be useful to be able to specify file-level ownership. |
|
@bgrant0607 actually for tests, ownership of just parts of files is what we want. Or maybe a different mechanism. @fejta may be able to find people to do this. I'm about to send an email to the testing sig about this. |
|
@lavalamp Ok, though I would like to be able to use whatever mechanism to automatically assign PRs and issues. |
|
Yes, the whole idea would be auto-flaky test issue assignment. Worst case, On Fri, Apr 15, 2016 at 4:57 PM, Brian Grant notifications@github.com
|
|
This might be a solution to the UI problem: http://gerrithub.io/ |
|
OK, it is my new mission in life to get some version of this merged ASAP. This feature has become pretty critical, and I think we should do the absolute minimum to get it merged. First, @eparis , thank you very much for doing this. The semantics you proposed look reasonable. I think the issue @bgrant0607 brought up about reusing LGTM is an interesting one. A scenario we need to consider is when someone wants to indicate that the PR looks fine to them, but they want someone else to also do a review (and thus it should not merge until the other person has looked at it). Under Brian's suggestion, if the first reviewer is also a valid approver, then they cannot use the phrase "LGTM" to indicate their satisfaction with the PR, because that will trigger a premature merge. There are obviously two solutions here: I think we should get rid of ok-to-merge completely. I don't think it has ever been useful. (I think it was intended to denote "I didn't check this PR's logic, but the PR at least seems non-malicious" or something like that.) I think we should also get rid of the LGTM label. The useful label will be Regarding the usability enhancements: I disagree that either of them is needed for the initial version of this feature. I think we should make it the PR assignee's responsibility to shepherd the PRs assigned to them through the process. So for example, if they are not a suitable approver, then they are responsible for reassigning the PR to a suitable approver (either instead of, or after, they have done a review). If this is done properly, then the user will be "in the loop" and doesn't need to understand the full policy (though I certainly don't object to having the bot automatically add a link to the policy when a PR is created). I am not a big fan of Brian's suggestion "we could post a message to the PR to describe the workflow and the escalation procedure, and list the leaf and trunk owners." The thing I worry about is that users will see that and immediately @-mention everyone in that (presumably very long) list, which will make being @-mentioned a completely useless signal, which would be very bad. Also it will go stale unless we edit it each time OWNERS files change. To be clear, I agree it would be useful to have tools to show approvers a list of PRs that they could approve, and (somewhat less so, since it's not hard to figure out once you know the policy) to show PR authors who is in the candidate approver pool. But I think that, for now, it's OK to lean on the PR's assignee to shepherd the PR through the approval process (and to reassign when they can't). Now that the auto-assigner is integrated with OWNERs this seems reasonable. WDYT? |
|
See also #869 |
|
There's discussion about ok-to-merge in kubernetes/kubernetes#24228 |
|
ping @eparis -- we'd really like to get this in... Could you comment on my last comment in this issue? |
|
@davidopp I'm also in favor of doing MVP, and iterating from there. I think the minimum is
As a separate task, someone can work on assigning approvers after the initial reviewer comments lgtm. |
|
What causes the PR to wait for a review if it is authored by an owner? On Tue, Jun 14, 2016, 19:10 Brian Grant notifications@github.com wrote:
|
|
@fejta Honor system? Nothing stops anyone with write access from applying |
|
If I'm an owner I expect to be able to send the review to a non owner and On Tue, Jun 14, 2016, 23:57 Brian Grant notifications@github.com wrote:
|
|
Previous workflow:
Proposed workflow:
It's pretty much the same, except that it bypasses github ACLs. We've already taken direct merge powers away from most people in kubernetes-maintainers. |
|
New flow sgtm. Thanks! On Wed, Jun 15, 2016, 09:54 Brian Grant notifications@github.com wrote:
|
|
@eparis What do you think of @bgrant0607 's suggestions here ? Do you think you can update this PR to do it? I am willing to take the first item he mentioned ("Add approvers to OWNERS files") |
|
I created a tracking issue for finishing this: #1389 |
| @@ -0,0 +1,180 @@ | |||
| /* | |||
| Copyright 2015 The Kubernetes Authors All rights reserved. | |||
|
Labelling this PR as size/L |
|
@hongchaodeng is working on OWNERS, but I don't think this particular PR is going anywhere, so closing. |
This PR is missing some key usability features.
I believe that before we turn this on, we REALLY need to solve those usability issues.