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 OWNERS for staging and api #44682

Merged
merged 1 commit into from
May 7, 2017

Conversation

smarterclayton
Copy link
Contributor

Part of #44420

  • api/ is a copy of pkg/api (same reasoning)
  • staging/ is the set of people who should be allowing new top level nested packages + the set of people who can change the staging machinery code

Open to changes to staging/ - very rarely changed. Added owners for the other items

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 19, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Apr 19, 2017
@k8s-reviewable
Copy link

This change is Reviewable

api/ is a copy of pkg/api (same reasoning)
staging/ is the set of people who should be allowing new top level
nested packages + the set of people who can change the staging machinery
code
staging/src/k8s.io/apimachinery/ is the set of core machinery people
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 19, 2017
@smarterclayton
Copy link
Contributor Author

@kubernetes/api-approvers @kubernetes/sig-api-machinery-misc adding OWNERS for staging/, api/, and staging/src/k8s.io/apimachinery/ based on similar roles in other packages. Please comment.

@dchen1107
Copy link
Member

Look good to me. Just one concern on how to make sure we do have the representative from the node on the package of api? Can we make sure bot can request proper reviewers from the node team on some of the API objects, like Pod, Node, etc.

@smarterclayton
Copy link
Contributor Author

Sure - probably should identify who that is and put them on both api and pkg/api

@dchen1107
Copy link
Member

I nominate this group of engineers representing the node related API: @derekwaynecarr (focusing on the resource management), @yujuhong (focusing on application lifecycle and management), @vishh (focusing on resource management), @timstclair (focusing on the monitoring and security for now) and myself as the catchall for now?

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 19, 2017

@smarterclayton: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins GCE e2e 32cbeaf link @k8s-bot cvm gce e2e test this

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.

@thockin
Copy link
Member

thockin commented Apr 20, 2017

I am happy with my assigned roles.

@smarterclayton
Copy link
Contributor Author

That's a lot of API approvers :)

@smarterclayton
Copy link
Contributor Author

Or were those just reviewers with yourself as the catchall approver?

@vishh
Copy link
Contributor

vishh commented Apr 20, 2017

@smarterclayton in the absence of a means to further narrow the approval scope, it would be helpful if everyone in @dchen1107's list had approver permissions.
Each of us work on and own various parts of the Node and Resource Management related APIs.

@dchen1107
Copy link
Member

@smarterclayton I am not proposing to add anyone in that group to the approver including myself. I just want to make sure one of them should be the reviewer for the corresponding node related API changes. But I am not sure if the current mechanism can enforce that.

@smarterclayton
Copy link
Contributor Author

We can't do magic, but all named folks are in the reviewers list for api and pkg/api.

@smarterclayton
Copy link
Contributor Author

As far as magic we should probably open a contrib-x issue if we want to expand the OWNER bot for that.

@smarterclayton
Copy link
Contributor Author

Other comments?

@smarterclayton smarterclayton added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Apr 28, 2017
@brendandburns
Copy link
Contributor

@k8s-bot cvm gce e2e test this

1 similar comment
@smarterclayton
Copy link
Contributor Author

@k8s-bot cvm gce e2e test this

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2017
@smarterclayton
Copy link
Contributor Author

No other comments, merging (based on general laziness)

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit a4a94d2 into kubernetes:master May 7, 2017
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. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants