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

sig-instrumentation: Add lilic to approvers #2800

Closed
wants to merge 1 commit into from

Conversation

brancz
Copy link
Member

@brancz brancz commented Jun 22, 2021

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/github-management Issues or PRs related to GitHub Management subproject labels Jun 22, 2021
@k8s-ci-robot k8s-ci-robot added sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 22, 2021
Copy link
Member

@logicalhan logicalhan left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, logicalhan

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@lilic
Copy link
Member

lilic commented Jun 22, 2021

Thanks! 🙇‍♀️ :)

@k8s-ci-robot
Copy link
Contributor

@brancz: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-org-test-all cfd603a link /test pull-org-test-all

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@ehashman
Copy link
Member

ehashman commented Jun 22, 2021

List needs to be alphabetized, which is why CI is failing.

@lilic is not an approver in k/k (where this list was pulled from---these are approvers in the kubernetes/ org) but is in kubernetes-sigs as KSM subproject owner. Should we be adding her here or to a kubernetes-sigs team?

I guess technically KSM is still in the kubernetes org but it's not supposed to be... :)

/hold
for discussion

(edit: basically, my intent for this team was that people have an alias that they can @ for a SIG Instrumentation approver, which would typically be in k/k, and @lilic doesn't have reviewer/approver there, so she'll end up getting non-actionable pings. There's some issues in kubernetes/steering#200 and kubernetes/community#5722 tracking better recognition for subproject owners.)

@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 Jun 22, 2021
@brancz
Copy link
Member Author

brancz commented Jun 22, 2021

This has nothing to do with kube-state-metrics, @lilic has reviewed plenty of instrumentation-related PRs and I want her to be able to approve any sig instrumentation-related PRs, because I trust whatever review she does, and it distributes the load. I don't know if this PR does that, but it's what I found searching through the org, if this PR does not reflect my intention, can you point out what needs to be different?

@ehashman
Copy link
Member

https://github.com/kubernetes/kubernetes/blob/6dd9deea3daa98e0003878544372b2c1b9e0e03d/OWNERS_ALIASES#L319-L336

Lili currently isn't listed as either an instrumentation reviewer or approver. We should fix that first before adding her to the GitHub team.

@ehashman
Copy link
Member

So, to be clear, this PR doesn't actually add any rights or responsibilities, it would just add another user to the GitHub team. I think as proposed it's a bit confusing for the reasons stated above. If we want to make OWNERS files changes, we should be following the community membership guidelines.

I would also like to see @lilic become an OWNER in k/k if that is something she's interested in doing. If that's the case, I think the next steps here are:

I have great respect and appreciation for @lilic's work, and I would be delighted for you to take on more responsibility in SIG Instrumentation.

@brancz
Copy link
Member Author

brancz commented Jun 23, 2021

For the record, Lili has been active in sig instrumentation for well over two years. I don't think us missing out on adding her to be a reviewer long ago is totally our mistake, and I think it's wrong not to acknowledge that mistake. I think this type of reaction makes us loose community members.

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. area/github-management Issues or PRs related to GitHub Management subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. 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.

5 participants