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

Clean up docker compose file for selfhosting #1022

Merged
merged 6 commits into from
May 25, 2023
Merged

Clean up docker compose file for selfhosting #1022

merged 6 commits into from
May 25, 2023

Conversation

Robin-Sch
Copy link
Contributor

@Robin-Sch Robin-Sch commented May 6, 2023

Removed nginx container (might want to put it back?)

I think the majority of people who will selfhost plane are selfhosting more than just plane, and this way the nginx container doesn't conflict with their current setup (port 80 is most likely already in use).
Maybe show/guide on how to setup a simple nginx server (in case they haven't done so)?

Changed spacing to 4 spaces for better readability

Changed the order, first the important stuff (plane) and later the database/redis

For example, to upgrade to a different version or change port(s) you don't have to scroll all the way down anymore
Also I don't think anyone will ever change something with the redis/postgres instance?

All containers are in the same format

First container_name, then image, then restart etc..
Just standardization, and looks better

Removed links

Links are deprecated since compose version 2, all containers are in one docker network

Removed build from plane-api

Now you don't have to download the whole repo anymore, just for the api image

Removed ports from redis and postgresql

They can communicate freely with each other without exposing the port, increases the security since the postgres and redis instances aren't accessible from the outside

Removed PGDATA environment variable

It's the default one and by removing it's less clutter in the file

Renamed redis and db to plane-redis and plane-db

This way you know the redis and postgres containers belong to plane and aren't just named "redis" and "db"

Could still be improved

The environment variables could be included in the .env file to remove them from the compose file and making the compose file more readable and smaller see this
This way you can change all environment variables from the .env file and they aren't spread across two files.

#1019

Robin-Sch added 5 commits May 6, 2023 18:33
Removed nginx container (might want to put it back?)
Changed spacing to tabs for better readability
Changed the order, first the important stuff (plane) and later the database/redis
All containers should be in the same format (first container_name, then image, then restart, etc.).
Removed links because deprecated since compose version 2, all containers are in one docker network
Removed build from plane-api
Removed ports from redis and postgresql
Removed PGDATA directory because that's the default one
Renamed redis and db to plane-redis and plane-db
@vercel
Copy link

vercel bot commented May 6, 2023

@Robin-Sch is attempting to deploy a commit to the Plane Team on Vercel.

A member of the Team first needs to authorize it.

@Igorgro
Copy link

Igorgro commented May 7, 2023

Changed spacing to tabs for better readability

Use tabs instead of spaces is a bad idea for YAML files

The environment variables could be included in the .env file to remove them from the compose file and making the compose file more readable and smaller see this

Totally agree with that. Currently, compose file looks too excess because of environment variables (most of them are duplicated from .env file by directives like VAR: ${VAR})

@vihar
Copy link
Contributor

vihar commented May 7, 2023

Thank you, folks, for your contributions. We are still testing different options, and this temporary unstable fix is just one of them. Our team is actively working on finding a permanent solution. We appreciate your contribution and will review it and merge it soon.

@sturnclaw
Copy link

Removed ports from redis and postgresql
They can communicate freely with each other without exposing the port, increases the security since the postgres and redis instances aren't accessible from the outside

I'd like to second this change - because the nginx instance and all of Plane's services are on the same Docker network, binding host ports is not required for communication between services. Exposing these services via bound host ports represents a security issue in single-server production environments; only the nginx proxy port needs to be accessible outside of the docker network for upstream traffic.

@Robin-Sch
Copy link
Contributor Author

Robin-Sch commented May 8, 2023

Use tabs instead of spaces is a bad idea for YAML files

My bad, I changed it to 4 spaces after I noticed tabs not working (that's why the multiple fix spacing commits)

@Robin-Sch
Copy link
Contributor Author

Robin-Sch commented May 8, 2023

I'm not sure which direction you want to go, include the nginx container or not. Afaik, many, if not all selfhosted services don't include a nginx container and just expect people to be already running their own reverse proxy.
However, if you're going to include the nginx container, it should also be documented that if you're already running a reverse proxy, that you should proxy api.example.com to port 8000 and plane.example.com to port 3000 and remove the nginx container (or something like that?).

Personally, I think it's the best to leave the container out, but yeah, let me know what you think of it.

@pablohashescobar
Copy link
Collaborator

Hi @Robin-Sch, thanks for your contribution. We will keep the nginx container and update the documentation for the people already running their own reverse proxy.

@pablohashescobar
Copy link
Collaborator

Merging this off, thank you for you contribution @Robin-Sch. Will update the documentation for users with and without nginx.

@pablohashescobar pablohashescobar merged commit e526a01 into makeplane:develop May 25, 2023
@Robin-Sch
Copy link
Contributor Author

#1019 (comment)

See @require-dev's suggestion too

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

Successfully merging this pull request may close these issues.

5 participants