4.0.0.docker #656
4.0.0.docker #656
Conversation
… setup at build or runtime
|
Hello, the checks above (codacy) does not check the commit at all, it checks some core files I didn't change at all... |
merge ryanhoddy 4.0.0 new commits
|
Yea you can ignore the codacy checks, that hasn't been configured correctly yet anyway. And sorry for the delay on reviewing this, my plan was to review it after I was done with all the other 4.0.0 code i've been working on (which is quite a lot). Currently I have 2 problems, 1) I'm busy with other code, and 2) I'm not that familiar with docker configurations right now to even decide how this should all work. I do however appreciate the work you've done and thank you for your time. Again, sorry that I'm so slow to respond. Lots of work and little time. |
|
Hi, I updated the PR to remove apache and use directly php artisan serve. |
Dockerfile
Outdated
| USER $user | ||
|
|
||
| HEALTHCHECK --interval=5m --timeout=3s --start-period=10s \ | ||
| CMD curl -f http://localhost/ || exit 1 |
There was a problem hiding this comment.
CMD is serving port 8000 but health check is 80. Typo?
There was a problem hiding this comment.
I wanted to check it this morning but it doesn't build anymore: the 'ammarkannas/laravel-bbcode-parser' package seems to have disappeared from github and the docker build now fails on composer install... :(
docker-compose.yml
Outdated
| version: "3.7" | ||
| services: | ||
| app: | ||
| image: ghcr.io/leolivier/fcms:artisan |
There was a problem hiding this comment.
This PR provides a Dockerfile but instead downloads an image from google container registry.
app should build from the provided Dockerfile.
There was a problem hiding this comment.
ghcr.io is not Google's Registry, it is GitHub's (GHCR = GitHub Container Registry)
As said in the header of this file, this is an example and this is also what I explain in the specific README.docker.md file by the way ;)
The image name is mine because it's the one I generated myself, it should become ghcr.io/ryanhowdy/fcms:artisan as soon as Ryan generates an "official" one.
And as explained, if users want to generate their own one, they'll need to update the image name accordingly like I did it myself.
There was a problem hiding this comment.
If a user is using docker compose, it means they already have docker and can build it as part of compose up. The image itself should just be a distribution.
Correcting the registry owned by GitHub is good, thanks. But the point still stands. It doesn’t really make sense to provide an image where the Dockerfile exists in a compose setup. Best to reduce 3rd party references. I don’t see how this could be merged as-is with the 3rd party image which is why I suggest switching it to build only.
If the owner decides to ship an image that’s a different matter but cross-repository references like this should be avoided for a PR proposed to be merged. This is just my suggestion. @ryanhowdy can decide how they want to proceed of coarse.
There was a problem hiding this comment.
Well, I disagree with the point of view that users should build the image themselves. If you want to make the life of your users easier, the owner should build the image himself each time there is a new release, put it in a registry (that's easy to do with Github Actions) and users won't have to even clone your github repo (which contains a non released version usually), you just need to provide an example of a docker-compose file like I did and an example of .env file that they can just copy/paste.
And, again, my docker-copose file was just an example and, I fully agree that the image name should be replaced by ryan's one. I will deliver again with his own repo instead of mine... (but it assumes Ryan will deliver an image ;)
|
|
||
| networks: | ||
| fcms-network: | ||
| driver: bridge |
There was a problem hiding this comment.
bridge networking is correct. It enables the two services to reach each other by DNS service name. e.g. http://app:8000
3 participants
Please cancel previous PR on Docker and use this one. This is exactly the same but on a clean branch on my side, not merged with french stuff.