Skip to content
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

add myself to OWNERs #1866

Merged
merged 2 commits into from
Nov 4, 2022
Merged

add myself to OWNERs #1866

merged 2 commits into from
Nov 4, 2022

Conversation

logicalhan
Copy link
Member

There are three current approvers, none from Google and I'm a chair for SIG instrumentation which sponsors this sub-project. I would like to be able to approve changes to alleviate review burden from the other three approvers.

/cc @ehashman @dashpole

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 20, 2022
@dashpole
Copy link

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2022
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2022
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2022
@dgrisonnet
Copy link
Member

dgrisonnet commented Oct 20, 2022

I am personally against adding a new approver without going through a normal reviewer period. This is for fairness reasons with the other long-time contributors as well as to make sure that approvers have grown enough to be able to make the correct long-term decisions for the project.

Also, although SIG Instrumentation owns its sub-projects, they have always been maintained separately from the SIG due to their nature of being very different from the in-tree codebase. If we feel like the SIG needs to take responsibility for the sub-projects, then I would be happy to add sig-instrumentation-reviewers/approvers to all the OWNERS files, but right now I feel like sub-projects owners should remain empowered to drive their project as they want.

I would like to be able to approve changes to alleviate review burden from the other three approvers.

As much as I agree that having more hands to help with reviews is great, the project is pretty healthy so I don't think we are in dire need of new approvers today. If SIG Instrumentation wants to step in and add approvers to projects that are single-handely maintained such as https://github.com/kubernetes-sigs/metrics-server/, https://github.com/kubernetes-sigs/prometheus-adapter, and https://github.com/kubernetes-sigs/custom-metrics-apiserver/, then I think that is great, but kube-state-metrics has been pretty healthy with 3 approvers from different companies so I don't think that is necessary, although some help with reviews would be more than welcome.

Note that I have 100% confidence that @logicalhan will make the right decisions for the project, but we have a process for becoming a sub-project approver and I don't think that being a SIG lead should make someone entitled to bypass it unless we are in a situation where a project is owned by a single company when that shouldn't be the case.

/hold

Leaving @fpetkovski @mrueg the chance to also voice their opinion.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 20, 2022
@mrueg
Copy link
Member

mrueg commented Oct 20, 2022

@logicalhan Thanks for starting the conversation! I agree with @dgrisonnet here. Feel free to reach out to the current maintainers in Kubernetes' Slack about your involvement and a path there. We want to ensure the vision and goals on the project are aligned.

@ehashman
Copy link
Member

Hey folks, hoping to clear up some of the confusion here.

@logicalhan is chair of SIG Instrumentation which is the sponsoring SIG for this subproject. From my understanding of SIG Governance and the approver ladder, leads of the SIG can request approver on any subproject, as we are typically granted administrative access by default, and any disagreements on the selection of approvers are ultimately escalated to us.

I believed that @logicalhan had discussed this request with all SIG Instrumentation leads and that we were all comfortable with him stepping up to bring more contributions into KSM, as he has many years of experience with the subproject. I was unaware of @dgrisonnet's concerns, but having read through I don't believe a SIG lead must go through a standard reviewer/approver process as though they are an untrusted/new contributor. SIG Instrumentation has never followed the contributor ladder guidelines to the letter in the past, and we've always leaned towards welcoming more contributions vs. having caution on adding new approvers.

I expect that as an approver, @logicalhan will work with the other subproject OWNERS (@fpetkovski @mrueg) and understand the existing roadmap and project priorities before setting direction, but based on his track record as an approver in other areas of Kubernetes I have all confidence he will do so.

@mrueg
Copy link
Member

mrueg commented Oct 20, 2022

Hey folks, hoping to clear up some of the confusion here.

@logicalhan is chair of SIG Instrumentation which is the sponsoring SIG for this subproject. From my understanding of SIG Governance and the approver ladder, leads of the SIG can request approver on any subproject, as we are typically granted administrative access by default, and any disagreements on the selection of approvers are ultimately escalated to us.

I don't disagree with that process and it's good that it exists for subprojects that need help. The approach taken here and the context initially provided, feels to me like entering the subproject from a position of power, which is not the style that I'm used to and I assume was not intended.

I believed that @logicalhan had discussed this request with all SIG Instrumentation leads and that we were all comfortable with him stepping up to bring more contributions into KSM, as he has many years of experience with the subproject. I was unaware of @dgrisonnet's concerns, but having read through I don't believe a SIG lead must go through a standard reviewer/approver process as though they are an untrusted/new contributor. SIG Instrumentation has never followed the contributor ladder guidelines to the letter in the past, and we've always leaned towards welcoming more contributions vs. having caution on adding new approvers.

Ideally maintainers should be involved or at least given a heads up, I was not aware of those discussions happening and am curious what triggered them, so we can improve on the subproject.

The project has got three active approvers. Balancing what @dgrisonnet mentioned about fairness towards other contributors, do you see any urgency to skip the regular process?

@ehashman
Copy link
Member

I don't disagree with that process and it's good that it exists for subprojects that need help. The approach taken here and the context initially provided, feels to me like entering the subproject from a position of power, which is not the style that I'm used to.
...
Ideally maintainers should be involved or at least given a heads up, I was not aware of those discussions happening and am curious what triggered them, so we can improve on the subproject.

This is definitely a miss, and I'm really sorry that it happened. I was the one who suggested that @logicalhan make this PR, as I understood one of the maintainers is currently on leave and he was offering to help out, and I did not anticipate pushback. I regret that it's come across this way and I don't think it's anyone's intent, although I now understand how it could have been perceived.

It's on us now to try to make this right.

The project has got three active approvers. Balancing what @dgrisonnet mentioned about fairness towards other contributors, do you see any urgency to skip the regular process?

I think our ability to bring on new contributors is predicated on our resources to mentor and grow other people. SIG Instrumentation has always prided ourselves on being very welcoming, and I want to maintain those values and ensure barriers are as low as possible to contribute to our SIG. If we have qualified people volunteering to help, I don't really want to turn them away or tell them to come back later---let's find a way to bring everyone in 😄

I'll reiterate that there is no "regular process" for someone already in a SIG Lead role: all of our contributor ladders are guidelines, not rules, geared towards a linear progression in the project for entirely new contributors, and I don't think it makes sense to ask a SIG Lead to go through all of that. To be clear, I am not claiming that leads should unilaterally PR themselves into OWNERS files. Everyone needs to be comfortable with this.

Ultimately, approver roles are about trust. It is crucial that all existing approvers of KSM support adding @logicalhan to the roster before this PR merges.

I regret that there's conflict over this, but I'm really glad we're having this conversation. Now that it's happened, I'd like to find a way to move forward---please let me know if there's anything I can do to help.

@dgrisonnet
Copy link
Member

dgrisonnet commented Oct 21, 2022

Ideally maintainers should be involved or at least given a heads up, I was not aware of those discussions happening and am curious what triggered them, so we can improve on the subproject.

💯 that is definitely my takeaway from these discussions, going forward I think that is the process we should follow as a SIG when onboarding new people in a project.

Ultimately, approver roles are about trust. It is crucial that all existing approvers of KSM support adding @logicalhan to the roster before this PR merges.

This is one of the reasons why we have contributors go through a reviewer period today, we are building this trust with our contributors by giving them more and more responsibilities over time. We are oftentimes revisiting our OWNERS file in ksm and are always looking for new contributors that could step up and help us.

I have personally been working with @logicalhan for a while now and as I mentioned earlier I have complete confidence and trust in him and can definitely see him being a great asset to the project if he were to become an approver. That said, others may have a different experience so although I can vouch for Han myself, our entire team of approvers needs to be in agreement and I want to make sure that if it's not the case we take the time that we need to build the trust that goes along with these responsibilities.

@logicalhan
Copy link
Member Author

Are you guys going to be at Kubecon? We can chat about this in person, it may be easier to discuss.

@mrueg
Copy link
Member

mrueg commented Oct 26, 2022

Are you guys going to be at Kubecon? We can chat about this in person, it may be easier to discuss.
Unfortunately not. I think there were some misunderstandings/different assumptions, most of it seems already resolved though. Happy to talk via video or chat on Slack for next steps.

OWNERS Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2022
@fpetkovski
Copy link
Contributor

/lgtm
/hold for @mrueg and @dgrisonnet to take a look.

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 4, 2022
@mrueg
Copy link
Member

mrueg commented Nov 4, 2022

/lgtm

as discussed on slack! Thanks for the follow-up and the clarification there @logicalhan!

@dgrisonnet
Copy link
Member

/lgtm
/unhold

Welcome to the team Han! 🙂

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 4, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, dgrisonnet, ehashman, fpetkovski, logicalhan, mrueg

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:
  • OWNERS [dgrisonnet,fpetkovski,mrueg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 32f08e3 into kubernetes:master Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants