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 'kubectl set image' #25509

Merged
merged 1 commit into from
May 20, 2016
Merged

Add 'kubectl set image' #25509

merged 1 commit into from
May 20, 2016

Conversation

janetkuo
Copy link
Member

@janetkuo janetkuo commented May 11, 2016 •
edited
Loading 

Add "kubectl set image" for easier updating container images (for pods or resources with pod templates).

Usage:

kubectl set image (-f FILENAME | TYPE NAME) CONTAINER_NAME_1=CONTAINER_IMAGE_1 ... CONTAINER_NAME_N=CONTAINER_IMAGE_N

Example:

# Set a deployment's nginx container image to 'nginx:1.9.1', and its busybox container image to 'busybox'.
$ kubectl set image deployment/nginx busybox=busybox nginx=nginx:1.9.1

# Update all deployments' nginx container's image to 'nginx:1.9.1'
$ kubectl set image deployments nginx=nginx:1.9.1 --all

# Update image of all containers of daemonset abc to 'nginx:1.9.1'
$ kubectl set image daemonset abc *=nginx:1.9.1

# Print result (in yaml format) of updating nginx container image from local file, without hitting the server 
$ kubectl set image -f path/to/file.yaml nginx=nginx:1.9.1 --local -o yaml

I abandoned the --container=xxx --image=xxx flags in the deploy proposal since it's much easier to use with just KEY=VALUE (CONTAINER_NAME=CONTAINER_IMAGE) pairs.

Ref #21648
@kubernetes/kubectl @bgrant0607 @kubernetes/sig-config

Analytics

@janetkuo janetkuo added priority/backlog Higher priority than priority/awaiting-more-evidence. area/app-lifecycle area/kubectl team/ux labels May 11, 2016
@janetkuo janetkuo added this to the v1.3 milestone May 11, 2016
@janetkuo janetkuo mentioned this pull request May 11, 2016
Closed
@k8s-github-robot k8s-github-robot added kind/old-docs size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 11, 2016
@janetkuo janetkuo 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 May 11, 2016
@bgrant0607
Copy link
Member

The main disadvantage of not using flags is that it makes supporting multiple resources impossible.

@janetkuo janetkuo force-pushed the kubectl-set-image branch from cab5663 to 191ca7d Compare May 11, 2016 22:47
@janetkuo
Copy link
Member Author

The main disadvantage of not using flags is that it makes supporting multiple resources impossible.

Why? Current implementation supports setting images of multiple resources. For example,

$ kubectl set image rc,deployment redis=redis --all
replicationcontroller "frontend" image updated
deployment "nginx-deployment" image updated

@janetkuo janetkuo force-pushed the kubectl-set-image branch from 191ca7d to ba3993d Compare May 11, 2016 22:58
@janetkuo
Copy link
Member Author

janetkuo commented May 11, 2016 •
edited
Loading

Per in-person discussion with @bgrant0607:

  1. This design works on multiple resources as expected.
  2. I'll change the behavior to error out instead of creating new container when the specified container name isn't found.

@janetkuo janetkuo force-pushed the kubectl-set-image branch 2 times, most recently from 0c38f00 to a5b69a0 Compare May 12, 2016 06:10
smarterclayton
smarterclayton reviewed May 13, 2016
View reviewed changes
hack/test-cmd.sh
kube::test::get_object_assert deployment "{{range.items}}{{$id_field}}:{{end}}" 'nginx-deployment:'
kube::test::get_object_assert deployment "{{range.items}}{{$deployment_image_field}}:{{end}}" "${IMAGE_DEPLOYMENT_R1}:"
# Set the deployment's image
kubectl set image deployment nginx-deployment nginx="${IMAGE_DEPLOYMENT_R2}" "${kube_flags[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add a test for wildcard.

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the expected behavior for wildcard? Wildcard in key (container name) or value (container image) or resource name?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was wildcard for container name so you could set image on every container in the pod. Just key I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added wildcard support and added test

@0xmichalis 0xmichalis mentioned this pull request May 17, 2016
Closed
@janetkuo janetkuo removed the priority/backlog Higher priority than priority/awaiting-more-evidence. label May 17, 2016
@janetkuo janetkuo force-pushed the kubectl-set-image branch from a5b69a0 to 5899e76 Compare May 17, 2016 20:05
@janetkuo
Copy link
Member Author

@smarterclayton PTAL

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2016
@janetkuo janetkuo force-pushed the kubectl-set-image branch from 5899e76 to 2c49680 Compare May 17, 2016 20:10
@janetkuo janetkuo removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2016
0xmichalis
0xmichalis reviewed May 17, 2016
View reviewed changes
pkg/kubectl/cmd/set/set_image.go
}

func hasWildcardKey(containerImages map[string]string) bool {
for k := range containerImages {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to traverse a map for a key, do you?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, you're right. Fixed

@smarterclayton
Copy link
Contributor

I had no more comments

@0xmichalis
Copy link
Contributor

squash down and this lgtm

@janetkuo janetkuo force-pushed the kubectl-set-image branch from b92b937 to 28a111e Compare May 17, 2016 20:54
@janetkuo
Copy link
Member Author

Squashed. Thanks!

@0xmichalis 0xmichalis added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2016
@janetkuo janetkuo mentioned this pull request May 18, 2016
Closed
2 tasks
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2016
@janetkuo janetkuo force-pushed the kubectl-set-image branch from 28a111e to 4332472 Compare May 19, 2016 04:23
@janetkuo janetkuo added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 19, 2016
@janetkuo
Copy link
Member Author

Bumping up priority from fixing test flakes #25756

@janetkuo janetkuo added the priority/backlog Higher priority than priority/awaiting-more-evidence. label May 19, 2016
@janetkuo janetkuo mentioned this pull request May 19, 2016
Closed
@janetkuo
Copy link
Member Author

@k8s-bot unit test this issue: #25891

@janetkuo
Copy link
Member Author

@k8s-bot unit test this issue: #25825

@janetkuo janetkuo mentioned this pull request May 19, 2016
Closed
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented May 20, 2016

GCE e2e build/test passed for commit 4332472.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 2c471bc into kubernetes:master May 20, 2016
@0xmichalis 0xmichalis mentioned this pull request Jun 12, 2016
Closed
@mengqiy mengqiy mentioned this pull request Sep 19, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@0xmichalis 0xmichalis

Labels
area/app-lifecycle area/kubectl lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
7 participants
@janetkuo @bgrant0607 @smarterclayton @0xmichalis @k8s-github-robot @k8s-bot @googlebot