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

Add a label command to kubectl. #4237

Merged
merged 1 commit into from
Feb 10, 2015
Merged

Add a label command to kubectl. #4237

merged 1 commit into from
Feb 10, 2015
+479 −0

Conversation

brendandburns
Copy link
Contributor

@bgrant0607 bgrant0607 self-assigned this Feb 7, 2015
@smarterclayton
Copy link
Contributor

Should this support bulk assignment from day one? Seems like this is the equiv style command as update.

@bgrant0607
Copy link
Member

Yes, it should support bulk assignment.

bgrant0607
bgrant0607 reviewed Feb 7, 2015
View reviewed changes
docs/kubectl.md

Usage:
```
kubectl label [--overwrite] <resource> <name> <key-1>=<val-1> ... <key-n>=<val-n> [--resource-version=<version>] [flags]
Copy link
Member

Choose a reason for hiding this comment

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

We use <id> for all other commands. We should be consistent, and convert them all to <name> at some point (soon).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, not in this PR ;)

@brendandburns
Copy link
Contributor Author

I was thinking about a postfix minus sign for delete. eg. kubectl label pods foo bar- I'd prefer a prefix, but I don't think the flags library can handle that.

@brendandburns
Copy link
Contributor Author

If the remove label syntax is acceptable folks, I'll code that up and send this for review.

@j3ffml
Copy link
Contributor

j3ffml commented Feb 9, 2015

I was thinking about a postfix minus sign for delete. eg. kubectl label pods foo bar-

How would that work with removing multiple labels?

@brendandburns
Copy link
Contributor Author

@jlowdermilk
kubectl label pods foo bar- baz- blah- another=value removethis- add=this ...
--brendan

@brendandburns
Copy link
Contributor Author

what does bulk update mean in this context? Do we want to support a label query? Do we want to support labeling all resources in a collection?

This is kind of unlike update because we don't specify files to relabel.

I'd prefer to add bulk update in a separate PR.

@j3ffml
Copy link
Contributor

j3ffml commented Feb 9, 2015

Remove label and bulk update as separate PR sg. LGTM.

@smarterclayton
Copy link
Contributor

On Feb 9, 2015, at 3:46 PM, Brendan Burns notifications@github.com wrote:

what does bulk update mean in this context? Do we want to support a label query? Do we want to support labeling all resources in a collection?

I think selecting a set of things to make more selective (by adding labels) prior to adding a new thing that uses the old label may not be an uncommon use of this command
This is kind of unlike update because we don't specify files to relabel.

Hrm - it's like delete though, and delete lets you use a directory of config to select the items to delete.
I'd prefer to add bulk update in a separate PR.

I'm fine with it being a follow on that is to be done before 1.0
—
Reply to this email directly or view it on GitHub.

@brendandburns
Copy link
Contributor Author

Remove implemented.

@smarterclayton I 100% agree that it will be a common use case.

Promise to do 2 PRs before 1.0 (probably this week):

  • use a selector to select objects, update all objects matched.
  • load objects from a config directory

sg?

j3ffml
j3ffml reviewed Feb 9, 2015
View reviewed changes
pkg/kubectl/cmd/label.go
If --overwrite is true, then existing labels can be overwritten, otherwise attempting to overwrite a label will result in an error.
If --resource-version is specified, then updates will use this resource version, otherwise the existing resource-version will be used.

Examples:
Copy link
Contributor

Choose a reason for hiding this comment

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

Add remove label example.

@smarterclayton
Copy link
Contributor

SGTM

----- Original Message -----

Remove implemented.

@smarterclayton I 100% agree that it will be a common use case.

Promise to do 2 PRs before 1.0 (probably this week):

  • use a selector to select objects, update all objects matched.
  • load objects from a config directory

sg?

Reply to this email directly or view it on GitHub:
#4237 (comment)

bgrant0607
bgrant0607 reviewed Feb 10, 2015
View reviewed changes
pkg/kubectl/cmd/label.go
overwrite := util.GetFlagBool(cmd, "overwrite")
resourceVersion := util.GetFlagString(cmd, "resource-version")

obj, err := updateObject(client, mapping, namespace, name, func(obj runtime.Object) runtime.Object {
Copy link
Member

Choose a reason for hiding this comment

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

I didn't see a response to the bulk-update issue.

Example is here:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/kubectl/cmd/update.go#L57

Obviously, a resource-version precondition would be incompatible with a bulk update.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, comment didn't show in file view, but saw it in email. I'm fine with a separate PR.

@bgrant0607 bgrant0607 assigned j3ffml and unassigned bgrant0607 Feb 10, 2015
@bgrant0607
Copy link
Member

Reassigning to @jlowdermilk since he left comments that still look unresolved.

@brendandburns
Copy link
Contributor Author

PR updated to document the remove label syntax.

@j3ffml
Copy link
Contributor

j3ffml commented Feb 10, 2015

lgtm

j3ffml added a commit that referenced this pull request Feb 10, 2015
@j3ffml
Merge pull request #4237 from brendandburns/pd_fix
ed9761f 
Add a label command to kubectl.
@j3ffml j3ffml merged commit ed9761f into kubernetes:master Feb 10, 2015
@brendandburns brendandburns deleted the pd_fix branch August 7, 2015 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brendandburns @smarterclayton @bgrant0607 @j3ffml