Skip to content

Report image we can't pull and must be built#7052

Merged
ulyssessouza merged 1 commit intodocker:masterfrom
ndeloof:pull_can_build
Nov 29, 2019
Merged

Report image we can't pull and must be built#7052
ulyssessouza merged 1 commit intodocker:masterfrom
ndeloof:pull_can_build

Conversation

@ndeloof
Copy link
Copy Markdown
Contributor

@ndeloof ndeloof commented Nov 25, 2019

As we pull images, on NotFound but service has build definition, report required command to build the missing images.

Resolves #6464

cc @justinmchase

def pull_service(service):
strm = service.pull(ignore_pull_failures, True, stream=True)

if strm is None: # Attempting to pull service with no `image` key is a no-op
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if strm is None: # Attempting to pull service with no `image` key is a no-op
if not strm: # Attempting to pull service with no `image` key is a no-op

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

was by initial code, but can change this indeed

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@justinmchase
Copy link
Copy Markdown

Hey, thanks for this!

I was thinking about it and I just wanted to add a little more context, I know previously I said that just not producing an error in the case of a service with image+build would solve my case (and it does) but I did just want to add that it is kind of a coincidence because my image was named something that doesn't happen to match an image in the docker repository.

For example mine is:

service:
  image: ambassador
  build: .

There is no image in docker hub simply called ambassador so it fails to pull and then goes down this route. However if I had happened to name the image node for example... then it would succeed to pull and probably would not do as I expect.

I'm not sure what the right thing to do there is, but I figured you'd want to consider it. My immediate thought would be to allow local image names to have characters that are illegal for remote images so it could just never pull in this case, such as image: $node... but I'm not sure how wise that is. Or maybe just a flag on the service: local: true. Or maybe its just always recommended they use one called image: local and that is special cased, I don't know.

Anyway thank you and I think this is a good improvement and compromise.

Copy link
Copy Markdown

@justinmchase justinmchase left a comment

Choose a reason for hiding this comment

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

I'm not a python expert or setup to test it locally but from the perspective of the fix based on previous convo's it looks good.

👍

service.pull(ignore_pull_failures, silent=silent)
if len(must_build):
log.warning('Some service image(s) must be built from source by running:\n'
' docker-compose build {}'
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

@ndeloof
Copy link
Copy Markdown
Contributor Author

ndeloof commented Nov 25, 2019

there's not such thing as a "local" image. Images are by design built so they can be shared by registries. To avoid collision with a Dockerhub image, you should just always use your own registry names : justinmchase/ambassador of mycompany/ambassador

@ulyssessouza ulyssessouza merged commit e82d38f into docker:master Nov 29, 2019
@ulyssessouza
Copy link
Copy Markdown
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docker-compose pull fails if a service includes both image and build

4 participants