Add multi stage build for docker for dev env, and placehoder for cerleryworker in docker compose

[sergioisidoro]

Overhauls Docker setup

• Makes use of multi stage builds to get a development environment with docker (which installs testing libraries, and mounts the code in the container for auto-reload)
• Removes Nginx from base and dev builds, and only adds them to production docker-compose using compose extends - https://docs.docker.com/compose/extends/
• Updates documentation
• Adds celery worker to and redis to the docker-compose prod setup, but keeps them commented and ready for use

[JuniorJPDJ]

This seems very cool, but would also probably need instructions for maintainer how to handle images in repository and some info about compatibility as it seems like backwards compatibility is lost.

EDIT: Would also be cool if you could handle Codacy Static Code Analysis failed check.

[sergioisidoro]

@JuniorJPDJ I tried to keep backwards compatibility as much as I could, and I think I achieved it to some degree The last stage of the docker build is production, which means the default behaviour of docker build is to build the production image just as before

But the docker-compose up behaviour indeed now defaults to the development environment, but I think that's the usual behaviour when using docker-compose extends (ie, default to development, and specifying a production file)

I updated the docs of the docker compose section the best I could

I'll take a look at the Codacy issuse, although it was a pre-existing one. It just got flagged because I moved the line

[milesmcc]

I worry that this might be too complicated. I really like having one Docker image for Shynet, and it really helps with debugging. While I really appreciate this proposal, I don't think it's the right path for us at this point.

I'll leave this PR open for a bit more deliberation (and I'm totally happy to merge a simpler PR that just improves the Dockerfile), but I think that having so many different stages and environments just makes things too complicated.

[sergioisidoro]

I really like having one Docker image for Shynet

I don't think this really changed, since in the end the default is the Docker production image.

The alternative is to either pollute the production image with testing and development dependencies or make a duplicate Dockerfile for running tests (which also creates another Docker image).

I have multiple projects running in parallel with different dependencies and requirements, and Docker is my defacto development environment. Not adding testing deps would be a major blocker for me being able to contribute.

[milesmcc]

It's a tough call, but I think this approach is too complex. Let's just put the development dependencies in the final image. It's only a few megabytes of difference (this is what I do for the debug toolbar, and I think it's fine).

I'd rather have a single Docker image to work with than two different versions. I'm also concerned about making it harder for new users to setup Shynet for the first time.

Thanks for working with me on this one.

[sergioisidoro]

I'm sorry @milesmcc, let me just be clear about some points before we close this PR:

• The default behaviour of docker build is the same as before. It will output the same image as before without any changes, and the people who use pure docker will have to make no changes to their setup.

• This PR does make some changes on the docker-compose setup. The setup of having docker-compose.yaml + docker-compose.override.yaml for development and a docker-compose.production.yaml for production is a very standard practice seen in thousands of repositories, documented in the official docker repository. It's the recommended setup to use docker-compose for development and production.

I can understand that having multiple docker-compose files can be, however, a bit intimidating. So we could instead have a single docker-compose.production.yaml for production, and have docker-compose.yaml for development? (we would remove the override)

Finally, I must ask. Is this problem about having multiple docker-compose files, or multiple stages of the docker build?

PS: "It's only a few megabytes of difference" I would add that adding testing and dev dependencies to the production image also adds security issues, such as accidentally enabling debugging and testing tools in production.

[milesmcc]

My concern is less with the complexity of the build, though that's certainly a consideration. (Currently the builds take 30m+, which is fine but less than ideal.) Instead, my concern is more with the complexity of the setup.

Would it be possible to keep docker-compose.yml for the production image, and then add a separate docker-compose.development.yml for development?

[sergioisidoro]

I can get behind that, and go for a simpler setup for now.

The benefit of this setup is that both the development compose file docker-compose.override.yaml and docker-compose.production.yaml inherit the base structure from the base docker-compose.yaml, reducing the probability of them diverging.

The downside of having 2 separate docker-compose files will be having to keep 2 separate "hard-copies" of docker-compose.yaml, which might end up diverging and not accurately reflect production when developing locally. But if that's that's an accepted tradeoff for simplicity, let's go with it.

To summarise:

• I will remove the multi stage build. I might need to also make root the production container user, because I remember having issues with running development tools as the appuser due to docker volume and file ownership.

• Remove inheritance of docker-compose.yaml and just make 2 hard copies of docker-compose

[milesmcc]

Sounds good. If possible, I'd really prefer to not run as the root user so that the risks of a container escape are smaller. I do wish there was a way for docker-compose.development.yml to override docker-compose.yml (to reduce duplication), but if there's no way to do that, oh well.

[sergioisidoro]

I do wish there was a way for docker-compose.development.yml to override docker-compose.yml

Well, the only 2 ways: either specifying 2 files with the -f flag, or with the override.

I have an idea. What if we keep a docker-compose.override.yaml in .gitignore and ask people to do cp docker-compose.development.yml docker-compose.override.yml so that they can override the docker compose easily.

The downside is that everytime we update the docker-compose.development.yml it won't reflect immediately on developers envs and they will have to re-copy it

[milesmcc]

I think it's ok to make it slightly more complicated for developers (i.e., ok to specify two files with -f) -- really I just want to make sure that the correct thing to do for the production deployment is also the simplest.

[milesmcc]

@sergioisidoro thanks again for this PR! I just wanted to check whether you think the approach where developers use -f to override makes sense?

[sergioisidoro]

Hi, sorry for the delay in returning to this topic.

In my opinion, using the -f flag make more sense in the environment where you type the command less often. I think that is why most docker setups opt for crating a separate docker compose like docker-compose.prod.yml as an override of the base file, or as a copy of the file itself.

If you think that for now it's better to make it slightly more complicated for developers then yes, I think the -f would be reasonable.

Something that also came to mind while writing this is that there starts to be a growing number of configurations:

• With / without Ngnix
• With / without celery

So using the -f in production continues to make a bit more sense to me in order to support those usecases, like so:

# Simple setup
docker compose up -f docker-compose.production.yml 

# with celery 
docker compose up -f docker-compose.production.yml -f docker-compose.celery.yml 

# with nginx 
docker compose up -f docker-compose.production.yml -f docker-compose.nginx.yml 

# with nginx + celery
docker compose up -f docker-compose.production.yml -f docker-compose.nginx.yml -f docker-compose.celery.yml 

[milesmcc]

Yeah, the approach you outlined does make sense. Let’s go with that.

[milesmcc]

Closing due to inactivity. Please feel to reopen if you're interested in implementing these changes.