laravel / sail

Docker files for running a basic Laravel application.
https://laravel.com/docs/sail
MIT License
1.7k stars 477 forks source link

Overwriting of images with multiple projects #649

Closed crazy252 closed 10 months ago

crazy252 commented 10 months ago

Sail Version

1.25.0

Laravel Version

10.27.0

PHP Version

8.2.14

Operating System

Windows (WSL)

OS Version

Ubuntu 22.04.3 (5.15.133.1-microsoft-standard-WSL2)

Description

Since docker version 4.26 the build and image definition are in conflict. With the new docker version, docker will overwrite the docker image of image. The docs for docker compose are not really clear here, what docker will do. This behavior is also with the new docker version 4.26.1. So maybe this is intented.

If you start the container, docker compose will print out the correct name for the container. But if you look in docker desktop, there is no container with this tag and the container in the image definition is in use.

The following part is the problem:

services:
    app:
        build:
            context: ./vendor/laravel/sail/runtimes/8.2
            dockerfile: Dockerfile
            args:
                WWWGROUP: '${WWWGROUP}'
        image: sail-8.2/app

If you remove the image definition, the problem is gone. And the image is correctly tagged after the build.

Is there are reason why this image definition is present with the build definition? I don't see any reason why, because the there is a build definition. So the image should never be used?

Steps To Reproduce

I testet this with laravel 8, 9 and 10 with different versions of sail on windows (wsl).

driesvints commented 10 months ago

I have no idea sorry. I do wonder what will happen if we were to remove it. I think it would break things for others maybe.

github-actions[bot] commented 10 months ago

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

crazy252 commented 10 months ago

I found the overwriting behavior in the docker compose docs: https://docs.docker.com/compose/compose-file/compose-file-v3/#build "If you specify image as well as build, then Compose names the built image with the webapp and optional tag specified in image".

So if you have multiple installations of laravel with sail, each image overwrites the "master" image. If you have one installation with octane, you have modified the image. If you start it, the docker image is not the image with octane. And if you rebuild the image in the octane project, you will overwrite all other projects with the same image because it is tagged as sail-8.2/app.

azzazkhan commented 10 months ago

@crazy252 I guess you can add custom tags to image if you're going to modify them like sail-8.3/app:octane.

remarkusable commented 10 months ago

Is there are reason why this image definition is present with the build definition? I don't see any reason why, because the there is a build definition. So the image should never be used?

@crazy252 since in Sail we custom build the laravel.test service image. the reason the image specified is so that if you have multiple Laravel Project/Docker Compose service that uses the same image name, we only need to build the image once, which makes sail up on the N-th projects will be much faster since the image is already built.

in the case you have different need on certain project, for example on one project you need Octane (with OpenSwoole/RoadRunner) you can set different image name as @azzazkhan suggested, for example sail-8.3/app:octane and other project can use the standard PHP CLI image sail-8.3/app:php-cli

crazy252 commented 10 months ago

@azzazkhan @remarkusable thank you for the explanation with the tags of the image definition.

I think this should be in the docs of sail or octane.

driesvints commented 10 months ago

We'd appreciate a PR to the docs. Thanks for all the help here all 👍