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

Implement kubectl annotation update command #11779

Merged
merged 1 commit into from
Aug 5, 2015
Merged

Implement kubectl annotation update command #11779

merged 1 commit into from
Aug 5, 2015
+1,182 −31

Conversation

janetkuo
Copy link
Member

Fixes #11486

# Update pod 'foo' with the annotation 'description' and the value 'my frontend'.
# If the same annotation is set multiple times, only the last value will be applied
$ kubectl annotate pods foo description='my frontend'

# Update pod 'foo' with the annotation 'description' and the value 'my frontend running nginx', overwriting any existing value.
$ kubectl annotate --overwrite pods foo description='my frontend running nginx'

# Update all pods in the namespace
$ kubectl annotate pods --all description='my frontend running nginx'

# Update pod 'foo' only if the resource is unchanged from version 1.
$ kubectl annotate pods foo description='my frontend running nginx' --resource-version=1

# Update pod 'foo' by removing an annotation named 'description' if it exists.
# Does not require the --overwrite flag.
$ kubectl annotate pods foo description-

cc @bgrant0607

@k8s-bot
Copy link

k8s-bot commented Jul 23, 2015

GCE e2e build/test passed for commit 6bd62938aa05814e7e475867a0278bd1caa6d654.

@nikhiljindal
Copy link
Contributor

Needs rebase

@janetkuo janetkuo force-pushed the add-kubectl-annotation branch from 6bd6293 to 8329e4e Compare July 24, 2015 22:17
@janetkuo
Copy link
Member Author

Code rebased.

@k8s-bot
Copy link

k8s-bot commented Jul 24, 2015

GCE e2e build/test passed for commit 8329e4e97d4fb283f50eb7f26f83282e00398639.

@bgrant0607
Copy link
Member

Thanks!

"annotate", a verb, would make a better command name.

Please use a "description" annotation as an example.

Shell comments start with #. (kubectl label has the same bug, I see)

cc @deads2k @jlowdermilk for code reviews

deads2k
deads2k reviewed Jul 27, 2015
View reviewed changes
pkg/kubectl/cmd/annotation.go
}

// validateNoAnnotationOverwrites validates that when overwrite is false, to-be-updated annotations don't exist in the object annotation map (yet)
func validateNoAnnotationOverwrites(meta *api.ObjectMeta, annotations map[string]string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be more user-friendly to report all the collisions at once instead of having the user fix them one at a time. This could also encourage the user to --overwrite with the intent of clobbering annotationA, but also end up clobbering annotationB.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@janetkuo janetkuo force-pushed the add-kubectl-annotation branch from 8329e4e to a67a2f1 Compare July 27, 2015 18:27
@k8s-bot
Copy link

k8s-bot commented Jul 27, 2015

GCE e2e build/test passed for commit a67a2f19be0541eda262721cb44279e9e5c9c9af.

@janetkuo janetkuo force-pushed the add-kubectl-annotation branch from a67a2f1 to 1bbfc8a Compare July 29, 2015 21:55
@janetkuo
Copy link
Member Author

@deads2k: code updated based on all above feedback, including refactoring the code to decouple command framework from business logic. PTAL. Thanks!

cc @lavalamp

@k8s-bot
Copy link

k8s-bot commented Jul 29, 2015

GCE e2e build/test passed for commit 1bbfc8ae2e0234099b9612507c3d52ff939b4232.

@janetkuo janetkuo added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 29, 2015
@janetkuo janetkuo force-pushed the add-kubectl-annotation branch from 1bbfc8a to 2c5b56b Compare July 29, 2015 23:17
@k8s-bot
Copy link

k8s-bot commented Jul 29, 2015

GCE e2e build/test passed for commit 2c5b56bfa81fcfbc356d572a5e2358c18ec3c6df.

@bgrant0607 bgrant0607 removed their assignment Jul 30, 2015
deads2k
deads2k reviewed Jul 30, 2015
View reviewed changes
pkg/kubectl/cmd/annotation.go
}

// parseAnnotations retrieves new and remove annotations from annotation args
func parseAnnotations(annotationArgs []string) (newAnnotations map[string]string, removeAnnotations []string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could return the error that "validate" would produce during the initial parse: https://github.com/GoogleCloudPlatform/kubernetes/pull/11779/files#diff-051990852b771d2bbf3fcd3209a24063R208

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@janetkuo janetkuo force-pushed the add-kubectl-annotation branch from 2c5b56b to f3bf8ed Compare July 31, 2015 17:38
@k8s-bot
Copy link

k8s-bot commented Jul 31, 2015

GCE e2e build/test passed for commit f3bf8edc4a893a8fb5575baff048b990e1ccacc4.

caesarxuchao
caesarxuchao reviewed Aug 3, 2015
View reviewed changes
pkg/kubectl/cmd/annotation.go
# Update pod 'foo' by removing an annotation named 'source_id' if it exists.
# Does not require the --overwrite flag.
$ kubectl annotate pods foo description-`
)
Copy link
Member

Choose a reason for hiding this comment

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

Hi @janetkuo, I don't quite understand this example. What is the 'source_id' here? Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be 'description'. I changed the example and forgot to change this comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@caesarxuchao
Copy link
Member

It seems this command allows the user to add the same annotation key multiple times in a single command, e.g.,

$ kubectl annotate pods/a a='A' a='AAA'

and the end result is we have an annotation a='AAA'.

Is this the expected behavior? If so, shall we document it?

@caesarxuchao
Copy link
Member

LGTM except a nit and a question. Thank you @janetkuo!

@janetkuo
Implement kubectl annotation update command. Refactor kubectl annotat…
7e63213 
…e to decouple command framework from business logic.
@janetkuo janetkuo force-pushed the add-kubectl-annotation branch from f3bf8ed to 7e63213 Compare August 3, 2015 21:36
janetkuo
janetkuo reviewed Aug 3, 2015
View reviewed changes
pkg/kubectl/cmd/annotation.go
limitranges (limits), persistentvolumes (pv), persistentvolumeclaims (pvc),
resourcequotas (quota) or secrets.`
annotate_example = `# Update pod 'foo' with the annotation 'description' and the value 'my frontend'.
# If the same annotation is set multiple times, only the last value will be applied
Copy link
Member Author

Choose a reason for hiding this comment

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

@caesarxuchao the behavior you mentioned above is expected. Notify the user about this behavior here.

@k8s-bot
Copy link

k8s-bot commented Aug 3, 2015

GCE e2e build/test passed for commit 7e63213.

@caesarxuchao
Copy link
Member

LGTM. Thank you!

@caesarxuchao caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2015
@alex-mohr
Copy link
Contributor

@k8s-bot test this [contrib/submit-queue: candidate for merging]

@k8s-bot
Copy link

k8s-bot commented Aug 5, 2015

GCE e2e build/test passed for commit 7e63213.

@alex-mohr
Copy link
Contributor

Automatic merge from SubmitQueue

alex-mohr added a commit that referenced this pull request Aug 5, 2015
@alex-mohr
Merge pull request #11779 from JanetKuo/add-kubectl-annotation
0e8020f 
Auto commit by PR queue bot
@alex-mohr alex-mohr merged commit 0e8020f into kubernetes:master Aug 5, 2015
@deads2k deads2k mentioned this pull request Aug 10, 2015
Closed
@janetkuo janetkuo mentioned this pull request Aug 24, 2015
Merged
@janetkuo janetkuo added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 14, 2015
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
area/kubectl 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. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@janetkuo @k8s-bot @nikhiljindal @bgrant0607 @caesarxuchao @alex-mohr @deads2k @googlebot