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

fix annotate.go single resource check #29319

Merged
merged 1 commit into from
Aug 4, 2016
Merged

fix annotate.go single resource check #29319

merged 1 commit into from
Aug 4, 2016

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Jul 20, 2016 •
edited by pwittrock
Loading 

Fix issue with kubectl annotate when --resource-version is provided.

When using kubectl annotate with a --resource-version on a resource, such as kubectl annotate pod <pod_name> --resource-version=1820 description='myannotation', the command fails with the error: error: --resource-version may only be used with a single resource.

Upon printing the output of resources that the annotate command receives from cli args, it prints: Resources:[pod <pod_name>]. In other words, it treats the name of the resource as a second resource. This PR addresses this issue by using the resource builder Singular flag to determine if only a single resource was passed.

@k8s-bot
Copy link

k8s-bot commented Jul 20, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

4 similar comments
@k8s-bot
Copy link

k8s-bot commented Jul 20, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jul 20, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jul 20, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jul 20, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@juanvallejo juanvallejo mentioned this pull request Jul 20, 2016
Merged
@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 Jul 20, 2016
@pwittrock
Copy link
Member

@adohe would you be willing to review this?

@adohe-zz
Copy link

Ok, I will review this today.

@adohe-zz
Copy link

@juanvallejo I think the Validate function can be simplify, also is there any reason we still keep this function? seems we can use validateAnnotations directly.

@juanvallejo
Copy link
Contributor Author

@adohe

I think the Validate function can be simplify

Hm, I'm not sure what you mean by this.

also is there any reason we still keep this function? seems we can use validateAnnotations directly.

It is to keep the same pattern (complete, validate, run) used in some other commands

@adohe-zz
Copy link

@juanvallejo for simplify, I mean:

func (o AnnotateOptions) Validate(args []string) error {
    return validateAnnotations(o.removeAnnotations, o.newAnnotations)
}

Ok, we keep this and follow this pattern.

@juanvallejo
Copy link
Contributor Author

@adohe Sounds good, I have gone ahead and simplified the Validate method. Thanks for your feedback.

@adohe-zz
Copy link

@juanvallejo you ever update this? It seems nothing changed.

@k8s-github-robot k8s-github-robot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. retest-not-required-docs-only and removed retest-not-required-docs-only labels Jul 28, 2016
@juanvallejo juanvallejo force-pushed the jvallejo_bugfix/single-resource-version-flag branch from feeeaf6 to 1e71359 Compare July 28, 2016 14:34
@juanvallejo
Copy link
Contributor Author

@adohe Sorry about that, forgot to push latest changes

@adohe-zz
Copy link

LGTM. Thanks @juanvallejo
@pwittrock please apply the ok-to-test label, I have no right to do this.

@pwittrock pwittrock added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2016
@pwittrock
Copy link
Member

@adohe Done. Thanks.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2016
@pwittrock pwittrock added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 2, 2016
@pwittrock pwittrock added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 2, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@adohe-zz
Copy link

adohe-zz commented Aug 4, 2016

ok to test. @pwittrock please help remove do-not-merge label.

@pwittrock
Copy link
Member

ok to test

@pwittrock pwittrock removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 4, 2016
@pwittrock
Copy link
Member

@adohe Alright, I think it is finally going to go through now :)

@k8s-bot
Copy link

k8s-bot commented Aug 4, 2016

GCE e2e build/test passed for commit 1e71359.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 8770b2e into kubernetes:master Aug 4, 2016
@juanvallejo juanvallejo deleted the jvallejo_bugfix/single-resource-version-flag branch August 4, 2016 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@adohe-zz adohe-zz

@pwittrock pwittrock

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@juanvallejo @k8s-bot @pwittrock @adohe-zz @k8s-github-robot @googlebot