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.

Sign up for GitHub

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

Jump to bottom

Move PDB controller and type ownership to SIG-Apps #45301

Merged
merged 1 commit into from
May 22, 2017
Merged

Move PDB controller and type ownership to SIG-Apps #45301

merged 1 commit into from
May 22, 2017

Conversation

erictune
Copy link
Member

@erictune erictune commented May 3, 2017

@erictune erictune requested a review from davidopp May 3, 2017 18:52
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 3, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels May 3, 2017
@erictune erictune mentioned this pull request May 3, 2017
Closed
ncdc
ncdc reviewed May 3, 2017
View reviewed changes
pkg/apis/policy/OWNERS Outdated
- mbohlool
- david-mcmahon
- jianhuiz
- sig-pr-reviews
Copy link
Member

Choose a reason for hiding this comment

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

missing -apps-?

ncdc
ncdc reviewed May 3, 2017
View reviewed changes
pkg/controller/disruption/OWNERS Outdated
- davidopp
- kargakis
- mwielgus
- sig-apps-pr-reviewers
Copy link
Member

Choose a reason for hiding this comment

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

should be sig-apps-pr-reviews (not reviewers)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Andy, fixed.

@krmayankk
Copy link

@erictune asking here, since i dont know where else to ask. How can i add myself as a reviewer for certain controllers . Do i need permission from someone to be added ?

@davidopp
Copy link
Member

davidopp commented May 3, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2017
@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 3, 2017
@erictune
Copy link
Member Author

erictune commented May 4, 2017

@krmayankk I'm looking into what the process is. It isn't documented well.

@0xmichalis
Copy link
Contributor

@erictune shouldn't the newly added groups in the ONWERS files be aliased here?

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. retest-not-required-docs-only labels May 4, 2017
@erictune
Copy link
Member Author

erictune commented May 4, 2017

PR Updated to use OWNER_ALIASES.
The use of that file is now documented in https://github.com/kubernetes/test-infra/pull/2562/files#diff-b59baac18a964b06d0f147ebc5f3c8cc

@erictune
Copy link
Member Author

erictune commented May 4, 2017

@krmayankk as best I can tell, the process is do 20 PRs and be invited by Approvers.
This is from: https://github.com/kubernetes/community/blob/master/governance.md#code-and-documentation-contributors

@erictune
Copy link
Member Author

erictune commented May 4, 2017

@k8s-bot verify test this

@erictune
Copy link
Member Author

erictune commented May 4, 2017

@krmayankk Also, you should feel free to do code reviews on any PR in an area that interests you, even if you are not an official reviewer.

@krmayankk
Copy link

@erictune thanks. I want to be added to important areas so that i get notifications on new PR. I think i have so far done more than 20 PRs and submitted a couple PR as well.

@erictune
Copy link
Member Author

erictune commented May 4, 2017

@k8s-bot tell me a joke

@erictune
Copy link
Member Author

erictune commented May 4, 2017

@k8s-bot verify test this

@davidopp
Copy link
Member

davidopp commented May 5, 2017

@k8s-bot gce etcd3 e2e test this
@k8s-bot verify test this
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: davidopp, erictune
We suggest the following additional approver: thockin

Assign the PR to them by writing /assign @thockin in a comment when ready.

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

@davidopp davidopp added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 5, 2017
0xmichalis
0xmichalis reviewed May 5, 2017
View reviewed changes
OWNERS_ALIASES Outdated
- smarterclayton
- soltysh
sig-apps-api-reviewers:
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not match what you add in pkg/apis/policy/OWNERS (sig-apps-api-reviewers vs sig-apps-api-approvers)

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2017
@erictune erictune added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2017
@davidopp
Copy link
Member

LGTM but might want to remove bprashanth

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2017
@erictune erictune added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels May 22, 2017
@erictune
Move PDB controller and type ownership to SIG-Apps
b17e3c1 
Created OWNERS_ALIASES called sig-apps-reviewers from the union of reviewers in:
 pkg/controller/{cronjob,deployment,daemon,job,replicaset,statefulset}/OWNERS
except removed inactive user bprashanth

Created OWNERS_ALIASES called sig-apps-api-reviewers as the intersection
of sig-apps-reviewers and the approvers from pkg/api/OWNERS.

Used those OWNERS_ALIASES as the reviewers/approvers for the disruption controller,
and API.
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 22, 2017 •
edited

@erictune: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins verification 9f2adae2153278c642b3437e7b56d01d821c3920 link @k8s-bot verify test this
pull-kubernetes-federation-e2e-gce b17e3c1 link @k8s-bot pull-kubernetes-federation-e2e-gce 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.

@erictune erictune added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2017
@erictune erictune removed the request for review from davidopp May 22, 2017 21:19
@erictune erictune added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2017
@erictune
Copy link
Member Author

I don't understand how an OWNERS file could cause just federation to fail, so merging.

@erictune erictune merged commit 12fbd82 into kubernetes:master May 22, 2017
@0xmichalis
Copy link
Contributor

@erictune that job is broken but has been disabled and does not block the queue: #45978 (comment)

@foxish foxish mentioned this pull request Jul 12, 2017
Merged
k8s-github-robot pushed a commit that referenced this pull request Jul 12, 2017
Merge pull request #48827 from foxish/move-more-e2e
4da48f1 
Automatic merge from submit-queue (batch tested with PRs 46738, 48827, 48831)

Moving disruption controller e2es to workload/

Based on #45301
Moving to track sig-apps in a single directory

cc @kubernetes/sig-contributor-experience-misc @kubernetes/sig-apps-misc @erictune @kow3ns @crimsonfaith91
@erictune erictune deleted the disrupt-approve branch August 8, 2017 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncdc ncdc ncdc left review comments

@0xmichalis 0xmichalis 0xmichalis left review comments

Assignees

@ncdc ncdc

@mml mml

@davidopp davidopp

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@erictune @k8s-reviewable @krmayankk @davidopp @0xmichalis @k8s-github-robot @k8s-ci-robot @ncdc @mml