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

Simplify List() signature in clients. #18075

Merged
merged 1 commit into from
Dec 3, 2015
Merged

Simplify List() signature in clients. #18075

merged 1 commit into from
Dec 3, 2015
+532 −650

Conversation

wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented Dec 2, 2015

This is a sibling PR to #17863

@krousey @lavalamp

@k8s-github-robot
Copy link

Labelling this PR as size/XXL

@k8s-github-robot k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 2, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 2, 2015

GCE e2e build/test failed for commit 1de0cd88f55c34cf7aebd4b078ff2589ec131113.

@wojtek-t wojtek-t force-pushed the only_list_options_in_list branch 2 times, most recently from aaa7b29 to f442deb Compare December 2, 2015 16:16
@wojtek-t wojtek-t closed this Dec 2, 2015
@wojtek-t wojtek-t reopened this Dec 2, 2015
@wojtek-t wojtek-t closed this Dec 2, 2015
@wojtek-t wojtek-t reopened this Dec 2, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 2, 2015

GCE e2e test build/test passed for commit aaa7b290868b86ba7b01b1737f5868dcba18315c.

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 2, 2015

@k8s-bot unit test this please

@k8s-bot
Copy link

k8s-bot commented Dec 2, 2015

GCE e2e build/test failed for commit f442deba6b1767180e51a9d6148dff6b99f90c14.

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 2, 2015

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Dec 2, 2015

GCE e2e test build/test passed for commit f442deba6b1767180e51a9d6148dff6b99f90c14.

@wojtek-t wojtek-t assigned lavalamp and unassigned krousey Dec 2, 2015
@lavalamp lavalamp assigned caesarxuchao and unassigned lavalamp Dec 2, 2015
@lavalamp
Copy link
Member

lavalamp commented Dec 2, 2015

@caesarxuchao Can you review this one? Thanks :)

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 2, 2015

@caesarxuchao - it would be awesome if you could do it. Thanks!

@caesarxuchao
Copy link
Member

Sure, I'll review it later today.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2015
caesarxuchao
caesarxuchao reviewed Dec 3, 2015
View reviewed changes
pkg/kubectl/rolling_updater.go
@@ -678,7 +680,9 @@ func AddDeploymentKeyToReplicationController(oldRc *api.ReplicationController, c
// Clean up any orphaned pods that don't have the new label, this can happen if the rc manager
// doesn't see the update to its pod template and creates a new pod with the old labels after
// we've finished re-adopting existing pods to the rc.
podList, err = client.Pods(namespace).List(labels.SelectorFromSet(selectorCopy), fields.Everything(), unversioned.ListOptions{})
selector = labels.SelectorFromSet(selectorCopy)
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, did you automate this kind of changes? If so, how did you do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

No - I did it manually.
[It was easy to fix it with sed when both selectors was Everything() - all other changes I did manually].

@caesarxuchao
Copy link
Member

LGTM. Thanks. It needs rebase.

@caesarxuchao caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 3, 2015
@wojtek-t wojtek-t force-pushed the only_list_options_in_list branch from f442deb to 6c8aa2d Compare December 3, 2015 07:27
@wojtek-t wojtek-t removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 3, 2015
@wojtek-t wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 3, 2015
@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 3, 2015

Trivial rebase - reapplying lgtm

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 3, 2015

GCE e2e build/test failed for commit 6c8aa2d3b61d7c24192d7aa7afc549d354cc0dd2.

@wojtek-t wojtek-t force-pushed the only_list_options_in_list branch from 6c8aa2d to 45e6d33 Compare December 3, 2015 08:06
@wojtek-t wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 3, 2015
@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 3, 2015

One more trivial rebase

@gmarek
Copy link
Contributor

gmarek commented Dec 3, 2015

@k8s-bot unit test this

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 3, 2015

unit-test failure is unrelated to this PR:
F1203 08:24:45.055467 17949 proxy.go:132] listen tcp 127.0.0.1:51245: bind: address already in use

@k8s-bot unit test this please

@k8s-bot
Copy link

k8s-bot commented Dec 3, 2015

GCE e2e test build/test passed for commit 45e6d331f71f492b868fdbe7fb3113407f923223.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2015
@wojtek-t wojtek-t force-pushed the only_list_options_in_list branch from 45e6d33 to 6dcb689 Compare December 3, 2015 08:54
@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 3, 2015

One more trivial rebase

@wojtek-t wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 3, 2015
@wojtek-t wojtek-t closed this Dec 3, 2015
@wojtek-t wojtek-t reopened this Dec 3, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 3, 2015

GCE e2e test build/test passed for commit 6dcb689.

gmarek pushed a commit that referenced this pull request Dec 3, 2015
Merge pull request #18075 from wojtek-t/only_list_options_in_list
ffdfc68 
Simplify List() signature in clients.
@gmarek gmarek merged commit ffdfc68 into kubernetes:master Dec 3, 2015
@wojtek-t wojtek-t deleted the only_list_options_in_list branch December 7, 2015 15:20
@krousey krousey mentioned this pull request Dec 7, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@caesarxuchao caesarxuchao

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@wojtek-t @k8s-github-robot @k8s-bot @lavalamp @caesarxuchao @gmarek @k8s-teamcity-mesosphere @krousey @googlebot