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

harden docker-compose #298

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0x4r45h
Copy link
Contributor

@0x4r45h 0x4r45h commented Feb 24, 2025

I did some research on community Namada-Indexer providers, and here’s what I found:

8 teams had Dragonfly exposed on the default port without any credentials.
4 teams had Postgres exposed on the default port with default credentials.
These PRs address these issues by:

  • Removing the binding of service ports to host ports (which wasn’t necessary).
  • Enforcing users to set a Postgres password
  • Adding a Postgres volume for persistent data storage.
  • Merging docker-compose-db.yml into the main compose file for better readability and reduced complexity.
  • Removing unused variables and Dockerfiles.

…ublic internet

merge docker-compose-db.yml to main compose file for better readability and reduce complexity
remove unused variables and Dockerfile
enforce user to set a password for postgres database
@Fraccaman
Copy link
Member

Fraccaman commented Feb 24, 2025

I dont get it, this is a docker compose used for development. I feel like infra operator should take this and modify it as they need? This is going to make development more difficult:

  • not having ports exposed by default makes debugging more difficult.
  • volumes during development are useless and lead only to developers getting confused on whats happening.
  • we have 2 docker compose because usually we work on a single service and we dont want to start the entire set of services together.

@0x4r45h
Copy link
Contributor Author

0x4r45h commented Feb 24, 2025

I dont get it, this is a docker compose used for development. I feel like infra operator should take this and modify it as they need? This is going to make development more difficult:

  • not having ports exposed by default makes debugging more difficult.
  • volumes during development are useless and lead only to developers getting confused on whats happening.
  • we have 2 docker compose because usually we work on a single service and we dont want to start the entire set of services together.

but people are using this in production.
we can create a separate compose file for development. e.g docker-compose-dev.yml
and instead of two separate compose files, we can use profiles to just run database+ Dragonfly.

OR, I can create a separate repository for production deployment using docker and use GitHub images from CI

@sirouk
Copy link
Contributor

sirouk commented Feb 24, 2025

I dont get it, this is a docker compose used for development. I feel like infra operator should take this and modify it as they need? This is going to make development more difficult:

  • not having ports exposed by default makes debugging more difficult.
  • volumes during development are useless and lead only to developers getting confused on whats happening.
  • we have 2 docker compose because usually we work on a single service and we dont want to start the entire set of services together.

I disagree. Perhaps there should be a docker-compose-dev.yml which continues to deploy as it does today, while we patch up docker-compose.yml for folks utilizing the template as-is.

@Fraccaman
Copy link
Member

sure, we can have two!

@Fraccaman
Copy link
Member

@0x4r45h can you split it into dev and prod?

@0x4r45h
Copy link
Contributor Author

0x4r45h commented Feb 25, 2025

@0x4r45h can you split it into dev and prod?

Yeah, sure, but I suggest keeping the docker-compose.yml for production. If we don’t, infra providers will have to pass the -f docker-compose-prod.yml flag to their commands, which could cause more confusion since most of them aren’t comfortable with Docker. For developers, we can alias it to a just command to make usage easier.
wdyt?

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.

3 participants