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

Adds flag modifying pull behavior for running and creating containers #1498

Merged
merged 6 commits into from May 6, 2019

Conversation

@zmackie
Copy link
Contributor

zmackie commented Nov 5, 2018

Should close issue moby/moby#34394

- What I did

  • This PR adds a new --pull flag to docker run and docker create, following the proposal in (moby/moby#34394)
  • Per this proposal, the flag is tristate:
    1. --pull=missing (this is the current behaviour and will be the default.)
    2. --pull=never
    3. --pull=always

- How I did it

  • Adds a new config field, createOptions.pull which forks the execution of container.createContainer to either pull the image if it does not exist at all locally --pull=missing, always try and update the image --pull=always, or never try and update the image, only using images that already exist on the local machine --pull=never

Both docker run and docker create can consume this flag.

- How to verify it

  • Run docker run and docker create with the flag. Notice that pulls images or does not, depending on the behavior specified in the flag. Running those commands without the flag should maintain the current behavior.

- Description for the changelog

Adds flag modifying pull behavior for running and creating containers (Pull if Missing, Pull Always, Pull Never)

- A picture of a cute animal (not mandatory but encouraged)
honey-badger

@zmackie

This comment has been minimized.

Copy link
Contributor Author

zmackie commented Nov 5, 2018

Hey docker folks. This is my first PR to the project (👋 ). It's likely deficient on tests (I especially wasn't sure about any kind of integration or e2e testing that needed to happen. Let me know what ya'll think.

@zmackie zmackie force-pushed the zmackie:34394-run-pull-flag branch from 0934964 to 58517d0 Nov 6, 2018
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 6, 2018

Codecov Report

Merging #1498 into master will increase coverage by 0.01%.
The diff coverage is 61.11%.

@@            Coverage Diff             @@
##           master    #1498      +/-   ##
==========================================
+ Coverage   56.75%   56.77%   +0.01%     
==========================================
  Files         309      309              
  Lines       21658    21668      +10     
==========================================
+ Hits        12292    12301       +9     
+ Misses       8469     8468       -1     
- Partials      897      899       +2
@lanoxx

This comment has been minimized.

Copy link

lanoxx commented Nov 12, 2018

Nice work. I hope the docker folks will merge this.

@zmackie zmackie force-pushed the zmackie:34394-run-pull-flag branch 2 times, most recently from a43abec to 05ecd71 Nov 14, 2018
@GordonTheTurtle

This comment has been minimized.

Copy link

GordonTheTurtle commented Nov 20, 2018

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "34394-run-pull-flag" git@github.com:Zanadar/cli.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354135168
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@zmackie zmackie force-pushed the zmackie:34394-run-pull-flag branch from f31fec0 to 053b7b4 Nov 20, 2018
@GordonTheTurtle GordonTheTurtle removed the dco/no label Nov 20, 2018
gerhard added a commit to thechangelog/changelog.com that referenced this pull request Dec 31, 2018
Using the pull command explicitly until the docker CLI supports this
natively. re docker/cli#1498

[#162511937]
@svenstaro

This comment has been minimized.

Copy link

svenstaro commented Jan 22, 2019

What's the work remaining in order to get this merged?

@zmackie

This comment has been minimized.

Copy link
Contributor Author

zmackie commented Jan 22, 2019

AFAIK nothing (but it’s been a while since I opened this so I may have forgotten). Maybe project folks have a different opinion?

@svenstaro

This comment has been minimized.

Copy link

svenstaro commented Jan 22, 2019

@GordonTheTurtle could you take a look?

@Morl99

This comment has been minimized.

Copy link

Morl99 commented Feb 11, 2019

@Zanadar can you try to increase the test coverage, you are slightly falling short, as per the above Check. I would love to see this feature merged, it is really useful!

@cpuguy83

This comment has been minimized.

Copy link
Collaborator

cpuguy83 commented Feb 14, 2019

Overall this looks good, but I think we can clean up the implementation a bit.

@cpuguy83

This comment has been minimized.

Copy link
Collaborator

cpuguy83 commented Feb 14, 2019

Also keep in mind pull-policy=always only really works for moving tags (such as latest).

Copy link
Collaborator

cpuguy83 left a comment

Should we error out early if pull=always and the pull failed?

@@ -194,31 +205,59 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerConfig
}

//create the container
response, err := dockerCli.Client().ContainerCreate(ctx, config, hostConfig, networkingConfig, opts.name)
var response container.ContainerCreateCreatedBody
if opts.pull == PullImageMissing { // Pull image only if it does not exist locally. Default.

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Feb 14, 2019

Collaborator

I think we can do this flow and it will clean up quite a bit:

warning, psuedo-code:

if PullImageAlways {
  pull()
}

err := createContainer()
if IsNotfound(err) && PullImageMissing {
  pull()
}

createContainer()

This comment has been minimized.

Copy link
@zmackie

zmackie Feb 16, 2019

Author Contributor

Latest commit improves the flow per suggestion, but I'm not sure I've got it quite right...will work at it a bit.

This comment has been minimized.

Copy link
@zmackie

zmackie Feb 17, 2019

Author Contributor

One thing I'm not certain on is whether we should error in the case that the image is not found locally and the "never pull" option is selected? In which case the flow becomes more like so:

if PullImageAlways {
  pull()
}

err := createContainer()
if err {
  if IsNotfound(err) && PullImageMissing {
    pull()
    createContainer()
  }
  return err // we won't be pulling so just return whatever err we got
}

This also saves trying to create the container twice in the "never pull" case (ie only try to create again in the case where we might pull).

@zmackie

This comment has been minimized.

Copy link
Contributor Author

zmackie commented Feb 16, 2019

Also keep in mind pull-policy=always only really works for moving tags (such as latest).

That's probably something that would have to be left to users, though, right? We can't necessarily know whether a tag is moving or not.

@cpuguy83

This comment has been minimized.

Copy link
Collaborator

cpuguy83 commented Feb 16, 2019

Right.

@zmackie zmackie force-pushed the zmackie:34394-run-pull-flag branch from f0b476c to a281095 Feb 17, 2019
@zmackie

This comment has been minimized.

Copy link
Contributor Author

zmackie commented Feb 17, 2019

Should we error out early if pull=always and the pull failed?

This is now handled in the first block: https://github.com/docker/cli/pull/1498/files#diff-477f60a1b609e77fdca573fd8bd7b0c9R210

@zmackie

This comment has been minimized.

Copy link
Contributor Author

zmackie commented Feb 17, 2019

@cpuguy83 Okay I cleaned things up a fair bit. I'd love a review pass on the tests and your thoughts on how we handle the error case when "never pull" is selected (see above).

@cpuguy83 cpuguy83 force-pushed the zmackie:34394-run-pull-flag branch from ee08ded to 483c53a May 6, 2019
@cpuguy83

This comment has been minimized.

Copy link
Collaborator

cpuguy83 commented May 6, 2019

I just pushed a rebase to see if that somehow magically fixes this.

@zmackie

This comment has been minimized.

Copy link
Contributor Author

zmackie commented May 6, 2019

woosh magic

@cpuguy83 cpuguy83 merged commit d88565d into docker:master May 6, 2019
9 checks passed
9 checks passed
ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/patch 61.11% of diff hit (target 50%)
Details
codecov/project 56.77% (+0.01%) compared to 3273c2e
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
dco-signed All commits are signed
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone May 6, 2019
@zmackie

This comment has been minimized.

Copy link
Contributor Author

zmackie commented May 6, 2019

🎆 🍾

@cpuguy83

This comment has been minimized.

Copy link
Collaborator

cpuguy83 commented May 6, 2019

Thanks for your patience all.

@bsamuels453

This comment has been minimized.

Copy link

bsamuels453 commented May 21, 2019

What's the difference between using --pull=always and --no-cache?

@cpuguy83

This comment has been minimized.

Copy link
Collaborator

cpuguy83 commented May 21, 2019

--no-cache is a docker build command and refers to build cache not image pulls.

@eduardobaitello

This comment has been minimized.

Copy link

eduardobaitello commented Jun 26, 2019

Now that this is merged, how to know in which stable CE version this flag/feature will become available?

Sorry if this question doesn't fit here.

@cpuguy83

This comment has been minimized.

Copy link
Collaborator

cpuguy83 commented Jun 26, 2019

@eduardobaitello This did not make it into 19.03 from what I can tell, so it will be the next major release after 19.03.

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jun 26, 2019

Yes, it missed the 19.03 train, but you can download a nightly build of the CLI to try it;

@srinathck

This comment has been minimized.

Copy link

srinathck commented Jul 18, 2019

Are these changes not going into docker for windows?

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jul 18, 2019

@srinathck they are, but download.docker.com does not have downloads for Windows binaries (you can install Docker EE on Windows server, or Docker Desktop on Windows client once there's a new release with this change)

@robbaman

This comment has been minimized.

Copy link

robbaman commented Jan 21, 2020

Is there a reason why the latest version of Docker Desktop on Windows still has version 19.03 of the CLI? According to this thread it seems 19.09 should've been worked on/ready for about 6 months already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.