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.

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

kubectl run add pull-policy flag to control image pull policy #30614

Merged
merged 1 commit into from
Aug 21, 2016

Conversation

adohe-zz
Copy link

@adohe-zz adohe-zz commented Aug 15, 2016

Add support for --image-pull-policy to 'kubectl run'

Fix #30493
@pwittrock @thockin ptal

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Aug 15, 2016
@adohe-zz adohe-zz force-pushed the run_pull_policy branch 2 times, most recently from ba0b7fc to 867c49a Compare August 15, 2016 11:04
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 15, 2016
@pwittrock pwittrock assigned pwittrock and mengqiy and unassigned j3ffml Aug 15, 2016
@pwittrock
Copy link
Member

@ymqytw Please shadow this code review.

@pwittrock
Copy link
Member

@adohe Thanks for the ping. Will review this today.

@pwittrock
Copy link
Member

@k8s-bot node e2e test this issue: #24451

@pwittrock
Copy link
Member

@adohe Started looking at this, will pick it up tomorrow.

@adohe-zz
Copy link
Author

@pwittrock thanks very much.

@pwittrock
Copy link
Member

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


pkg/kubectl/run.go, line 620 [r1] (raw file):

  imagePullPolicy := api.PullPolicy(params["image-pull-policy"])
  if len(imagePullPolicy) == 0 {
      imagePullPolicy = api.PullAlways

We should either:

  • Only set the imagePullPolicy if it is set, and leave it empty if unset (this will rely on server side defaulting)
  • Set the default imagePullPolicy value in the flag default so that it is better documented what the default is

I am leaning towards the former - e.g. don't set the imagePullPolicy here if the flag is empty.


pkg/kubectl/run.go, line 654 [r1] (raw file):

  imagePullPolicy := v1.PullPolicy(params["image-pull-policy"])
  if len(imagePullPolicy) == 0 {

Same comment here about defaulting.


pkg/kubectl/run.go, line 911 [r1] (raw file):

  imagePullPolicy := api.PullPolicy(params["image-pull-policy"])
  if len(imagePullPolicy) == 0 {
      imagePullPolicy = api.PullIfNotPresent

Why do we use a different default here than elsewhere? Why do we set client-side defaults here but not elsewhere?


pkg/kubectl/run_test.go, line 40 [r1] (raw file):

              "name":              "foo",
              "image":             "someimage",
              "image-pull-policy": "Always",

If we only explicitly set the value in the config when the flag is supplied, then we can get rid of most of these changes.


pkg/kubectl/cmd/run.go, line 108 [r1] (raw file):
Explain the unset behavior. e.g.

If left empty, this value will not be specified by the client and defaulted by the server.


pkg/kubectl/cmd/run.go, line 172 [r1] (raw file):

  if err := verifyImagePullPolicy(cmd); err != nil {
      return nil

Shouldn't this return the err?


Comments from Reviewable

@pwittrock
Copy link
Member

Review Pass Finished


Review status: all files reviewed at latest revision, 6 unresolved discussions.


Comments from Reviewable

@adohe-zz
Copy link
Author

adohe-zz commented Aug 16, 2016

Thanks @pwittrock, I will update it. Besides this reviewable tool looks great.

@adohe-zz
Copy link
Author

pkg/kubectl/run.go, line 911 [r1] (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Why do we use a different default here than elsewhere? Why do we set client-side defaults here but not elsewhere?

I have no idea why do this for BasicPod generator, but we did do it. I just respect the old behavior.

Comments from Reviewable

@adohe-zz
Copy link
Author

pkg/kubectl/run.go, line 620 [r1] (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

We should either:

  • Only set the imagePullPolicy if it is set, and leave it empty if unset (this will rely on server side defaulting)
  • Set the default imagePullPolicy value in the flag default so that it is better documented what the default is

I am leaning towards the former - e.g. don't set the imagePullPolicy here if the flag is empty.

Agreed.

Comments from Reviewable

@adohe-zz
Copy link
Author

pkg/kubectl/cmd/run.go, line 172 [r1] (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Shouldn't this return the err?

ohh~some stupid mistake I just made.

Comments from Reviewable

@pwittrock
Copy link
Member

Review status: all files reviewed at latest revision, 6 unresolved discussions.


pkg/kubectl/run.go, line 911 [r1] (raw file):
Alright, sounds good for now. Consider adding a comment stating we don't know why this is specificed:

// TODO: Figure out why this is set here, and whether we can make it consistent with the other places imagePullPolicy is set using the flag


Comments from Reviewable

@pwittrock
Copy link
Member

Yeah I really like the new tool. It is much more powerful and allows you to do diffs against arbitrary versions.


Review status: all files reviewed at latest revision, 6 unresolved discussions.


Comments from Reviewable

@adohe-zz
Copy link
Author

pkg/kubectl/run.go, line 911 [r1] (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Alright, sounds good for now. Consider adding a comment stating we don't know why this is specificed:

// TODO: Figure out why this is set here, and whether we can make it consistent with the other places imagePullPolicy is set using the flag

Done.

Comments from Reviewable

@adohe-zz
Copy link
Author

pkg/kubectl/cmd/run.go, line 172 [r1] (raw file):

Previously, AdoHe (TonyAdo) wrote…

ohh~some stupid mistake I just made.

Done.

Comments from Reviewable

@adohe-zz
Copy link
Author

@pwittrock Just update, ptal.

@mengqiy
Copy link
Member

mengqiy commented Aug 17, 2016

:lgtm:


Comments from Reviewable

@pwittrock
Copy link
Member

:lgtm:


Review status: 0 of 3 files reviewed at latest revision, 5 unresolved discussions.


Comments from Reviewable

@pwittrock pwittrock added 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. and removed release-note-label-needed labels Aug 17, 2016
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2016
@pwittrock
Copy link
Member

Reviewed 1 of 3 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@pwittrock
Copy link
Member

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

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

@adohe I thin there is a way for you to mark my comments as done in Reviewable. In the future would you see if this is possible?

@adohe-zz
Copy link
Author

@pwittrock yes, it should be possible to do that.

@k8s-bot
Copy link

k8s-bot commented Aug 21, 2016

GCE e2e build/test passed for commit ca315e3.

@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 Aug 21, 2016

GCE e2e build/test passed for commit ca315e3.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit d1ed6f5 into kubernetes:master Aug 21, 2016
@adohe-zz adohe-zz deleted the run_pull_policy branch August 21, 2016 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants