add sjenning and mrunalp to approvers and reviewers #505
Conversation
rphillips
force-pushed
the
add_sjenning_mrunal
branch
from
30ac4f9
to
8abd50d
Compare
OWNERS
Outdated
| @@ -9,6 +9,8 @@ filters: | |||
| - mfojtik | |||
| - marun | |||
| - tnozicka | |||
| - mrunalp | |||
| - sjenning | |||
|
|
|||
| # approvers are limited to the team that manages rebases and pays the price for carries that are introduced | |||
There was a problem hiding this comment.
@rphillips This comment suggest against addition as approvers.
There was a problem hiding this comment.
These individuals have had approval rights within Origin to perform approvals on this section of code before. Not allowing Seth or Mrunal approval rights here will greatly slow down the team, since it will be dependent on non-owners of the kubelet code to approve and review changes.
Was there a discussion on slack or the dev list about approval rights here?
There was a problem hiding this comment.
Not allowing Seth or Mrunal approval rights here will greatly slow down the team, since it will be dependent on non-owners of the kubelet code to approve and review changes.
The solution is to build kubelet out of another repo/branch. It is what networking team and oc team do. As long as o/k is the build source, we have this rule and are very hesitant to weaken it. We understand the pain though, totally.
There was a problem hiding this comment.
Not allowing Seth or Mrunal approval rights here will greatly slow down the team
Where has this actually happened?
I would claim that approvers here do a very good job to react quickly to approval requests. If approval is rejected, there should be good reasons. I would personally be curious where you had the impression in the past where we did not live up to the standard.
Given than maintenance of the fork is not done by @mrunalp or by @sjenning or by their teams, I'm inclined to leave the approval of patches outside of their domain in upstream kubernetes (owners files from kube are honored) to the teams who actually maintain the code. The origin codebase of old was much larger and the need to approve changes across areas other than the fork of kubernetes forced a wider set of approvers. |
|
/hold based on above. |
openshift-ci-robot
added
the
do-not-merge/hold
OWNERS
Outdated
| @@ -18,6 +20,8 @@ filters: | |||
| - mfojtik | |||
| - marun | |||
| - tnozicka | |||
| - mrunalp | |||
| - sjenning | |||
|
|
|||
rphillips
force-pushed
the
add_sjenning_mrunal
branch
3 times, most recently
from
83a8b95
to
19f1da6
Compare
|
As agreed in Slack. /lgtm |
openshift-ci-robot
added
lgtm
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
rphillips
force-pushed
the
add_sjenning_mrunal
branch
from
19f1da6
to
987aa53
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, rphillips, sttts The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Add mrunalp and sjenning to the approvers and reviewers.
cc @mfojtik @sjenning @mrunalp @deads2k