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

Revert "only pull images that can't build" #7039

Merged
merged 1 commit into from Nov 21, 2019

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Nov 21, 2019

#6494 fix is incorect

a service can define both image and build, and documentation gives some indications on how those are handled:

If you specify image as well as build, then Compose names the built image with the webapp and optional tag specified in image

If the image does not exist, Compose attempts to pull it, unless you have also specified build, in which case it builds it using the specified options and tags it with the specified tag.

unfortunately those indication are not command-specific and the pull command documentation doesn't cover this specific case.

Some use image to tag built services and later push images, which will be pulled by others. Some use image only as a "build cache" and then don't expect compose to pull those from registry. AFAICT this is the case for #6464 - can you please confirm @justinmchase ?

I also can see lack of clarification on the expectation when both build and image are set brings many ambiguities, see #3853

I propose we first revert change, so 1.25 will behave like 1.24 did, then work on better documentation on docker-compose pull and impacts of build and image combination for a service.

About #6464, --ignore-pull-failures should be enough to offer a reasonable workaround.

This PR reverts commit c6dd7da.

Resolves #7038
Resolves #6934

This reverts commit c6dd7da.

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

@ulyssessouza ulyssessouza left a comment

Choose a reason for hiding this comment

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

LGTM

@ndeloof ndeloof merged commit e6ec770 into docker:master Nov 21, 2019
@ndeloof ndeloof deleted the build_or_push branch November 21, 2019 14:26
@justinmchase
Copy link

justinmchase commented Nov 21, 2019

Some use image only as a "build cache" and then don't expect compose to pull those from registry.

Correct, in my case I'm using image and build such that the same image is shared between services but is built from local and never pulled from remote.

Though...

I propose we first revert change, so 1.25 will behave like 1.24 did, then work on better documentation on docker-compose pull and impacts of build and image combination for a service.

I would be much more in favor of not just adding documentation but actually fixing the issue (the right way).

@justinmchase
Copy link

So I don't understand this reversion, it looks like the bug was fixed and now you've just broken it again?

You said:

unfortunately those indication are not command-specific and the pull command documentation doesn't cover this specific case.

But this change that was just reverted did cover that case, no? For those two bugs listed here it appears that they are "by design" and were previously explained by the documentation. They should then simply call "docker-compose build" and their problems are solved.

I guess I would not have recommended reversion as the fix here, perhaps I could see adding an automatic build step for images that aren't already built but I think this reversion was probably a mistake.

@justinmchase
Copy link

justinmchase commented Nov 21, 2019

I think I understand... the folks with the issue are using the same file to pull from prod for local work instead of building but they can build it locally if they wanted to by explicitly using the build command.

One question about that, if you pull and then build will the build be skipped because you have the image already?

But if that's the case I would propose this, which I think would still work for these users but would also work for my use case:

  1. If you run docker-compose pull and it fails to find the image on remote...
    1. If the service with a missing image also has a build, or any other service referencing that image has a build...
      1. A non-error message is printed: "example... not found, skipping local image"
    2. Else it has no local build
      1. An error is produced: "example... error"

I noticed one of the users had an image such as image: oharastream/configurator:${TAG:-0.9.0-SNAPSHOT} which is interesting because they may be working on a version which hasn't yet been deployed to production in which case they would (after this reversion) get the error I reported if they tried to pull with an invalid take or snapshot value. However with the recommended change here they would get an "example... not found, skipping local image" non-error instead.

For bonus points a -b|--build-images (or whatever) flag could be introduced in this case to, instead of skipping, build local image which is missing from remote.

@ndeloof
Copy link
Contributor Author

ndeloof commented Nov 21, 2019

Some people indeed use image as a "build cache" so that docker-compose build is efficient even with suboptimal Dockerfiles, as long as you use the same image tag - so the use of image: foo/bar:${GIT_COMMIT} and such things.

Others do use image as tag for the image being built and pushed to a registry, assuming the same compose file can later be used to pull back on production servers, fully ignoring the build section.

Once we get 1.25.1 released I'll discuss with maintainers so we create better documentation on this topic and/or introduce explicit flags so one get full control on whats happening.

@justinmchase
Copy link

Sounds good thanks.

@ndeloof
Copy link
Contributor Author

ndeloof commented Nov 25, 2019

See #7052 as my proposed "better" (?) fix for this issue

@teohhanhui
Copy link

Some use image to tag built services and later push images, which will be pulled by others.

Yes, this is very important for our use case. We build images in CI so that a new user can pull them (to provide the best OOTB experience), while the user should also be able to build the images locally.

Thanks for reverting.

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