-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
fix parse resource in setting selector #47719
fix parse resource in setting selector #47719
Conversation
Hi @xilabao. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. 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. |
@k8s-bot ok to test |
54dadd2
to
116a079
Compare
/approve |
@@ -116,6 +116,7 @@ func (o *SelectorOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args [ | |||
mapper, _ := f.Object() | |||
o.mapper = mapper | |||
o.encoder = f.JSONEncoder() | |||
o.resources, o.selector, err = getResourcesAndSelector(args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it return if err != nil here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. thanks!
116a079
to
4f735a7
Compare
/assign @deads2k |
/unassign |
@fabianofranz I think that @juanvallejo was in here recently, but I can't seem to choose him from the list |
@deads2k will add my review, thanks |
hack/make-rules/test-cmd-util.sh
Outdated
@@ -2440,7 +2440,7 @@ run_service_tests() { | |||
# prove role=padawan | |||
kube::test::get_object_assert 'services redis-master' "{{range$service_selector_field}}{{.}}:{{end}}" "padawan:" | |||
# Set command to reset the selector back to the original one. | |||
kubectl set selector -f examples/guestbook/redis-master-service.yaml app=redis,role=master,tier=backend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth keeping this line, maybe also add a --local
test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. PTAL @juanvallejo
One comment, otherwise this lgtm |
4f735a7
to
21bd8bc
Compare
706ae3a
to
0ba41e7
Compare
/test pull-kubernetes-kubemark-e2e-gce |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, dims, xilabao Associated issue: 47718 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test pull-kubernetes-kubemark-e2e-gce |
/test pull-kubernetes-unit |
Automatic merge from submit-queue (batch tested with PRs 48196, 42783, 48507, 47719, 46138) |
@xilabao PR needs rebase |
Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
What this PR does / why we need it:
Which issue this PR fixes: fixes #47718
Special notes for your reviewer:
Release note: