Add bind mounts to docker compose dev setup + migration command override

spwoodcock commented 4 months ago

What type of PR is this? (check all applicable)

[ ] 🍕 Feature
[ ] 🐛 Bug Fix
[ ] 📝 Documentation
[ ] 🧑‍💻 Refactor
[ ] ✅ Test
[x] 🤖 Build or CI
[ ] ❓ Other (please specify)

Describe this PR

Bind mounts
- Currently there are no bind-mounts to local files inside dev containers.
- Meaning we have to build every time to run tests or migrations.
- Mounting the files makes it easier for: hot reload, running tests, running migrations.
I also removed the version param from docker-compose.yml files, as it's now obsolete.
Moved the upgrade command out of the tm-migration entrypoint. Allow for override via command: xxx in docker-compose.

Review Guide

Notes for the reviewer. How to test this change?

Checklist before requesting a review

📖 Read the Tasking Manager Contributing Guide: https://github.com/hotosm/tasking-manager/blob/develop/docs/developers/contributing.md
📖 Read the HOT Code of Conduct: https://docs.hotosm.org/code-of-conduct
👷‍♀️ Create small PRs. In most cases, this will be possible.
✅ Provide tests for your changes.
📝 Use descriptive commit messages.
📗 Update any related documentation and include any relevant screenshots.
🔠 Does this PR introduce or change any environment variables? If so, make sure to specify this change in the description.

[optional] What gif best describes this PR or how it makes you feel?

spwoodcock commented 4 months ago

I just rebased to develop.

Also added default values for POSTGRES_USER and POSTGRES_DB for the healthcheck in docker-compose.yml.

One of the annoying things about using a .env file named tasking-manager.env instead of .env is that we either need to specify with docker-compose --env-file tasking-manager.env ... or specify default for a standard docker-compose ....

Values in .env are automatically injected into docker-compose.yml, while vars in custom named .env files are not.

mahesh-naxa commented 4 months ago

I just rebased to develop.

Also added default values for POSTGRES_USER and POSTGRES_DB for the healthcheck in docker-compose.yml.

One of the annoying things about using a .env file named tasking-manager.env instead of .env is that we either need to specify with docker-compose --env-file tasking-manager.env ... or specify default for a standard docker-compose ....

Values in .env are automatically injected into docker-compose.yml, while vars in custom named .env files are not.

Yeah @spwoodcock i recently had to dig into this, As, Bash from techwd, had issues trying to set up tasking-manager on his server. He got this.

Despite setting the variables in the tasking-manager.env file. We are getting the following on running 'docker compose up --detach':
WARN[0000] The "POSTGRES_USER" variable is not set. Defaulting to a blank string.                                  
WARN[0000] The "POSTGRES_DB" variable is not set. Defaulting to a blank string.

The warning was due to the vars not setting up in compose file. Although not an issue as pg_isready -U "anything" -d "anything" would give the exact exit code, even if its an empty string. Basically it would just check if postgres server is up or not. For those setting up tasking-manager for the first time, this caused migration to fail as the database wasn't initialized on time. We have start_period in health-checks , I thought it acted like a grace-period(that doesn't perform health check for this period of time), but this time flag is just for discarding health-checks results before that time. But Our's one succeeds anyway, so it will pass at the first second it gets.

This caused migration container to fail, resulting in backend service to be Created but not Started.

I think we should replace this with something that does actual query, i have seen examples like this one: https://github.com/docker-library/healthcheck/blob/master/postgres/docker-healthcheck

Your thoughts?

spwoodcock commented 4 months ago

You can use depends_on in docker compose for this purpose 😄

With various conditions like service_healthy. Check the FMTM docker compose files for examples 👍

Adding of the default vars in this PR will also fix the error the user above was having

prabinoid commented 4 months ago

Related Issue #6498

mahesh-naxa commented 4 months ago

You can use depends_on in docker compose for this purpose 😄

With various conditions like service_healthy. Check the FMTM docker compose files for examples 👍

Adding of the default vars in this PR will also fix the error the user above was having

Agreed depends_on is the way it should avoid these situation, and we have it in tasking-manager docker setup. But, here db service is presumed healthy due to the health checks that we have, its similar to fmtm right now. here. But when user sets up the project for the 1st time, The process flows like this:

db service is started.
migration container is created but not started, since it depends_on db container to be healthy.
db container is healthy, as pg_isready -U ${FMTM_DB_USER:-fmtm} -d ${FMTM_DB_NAME:-fmtm} exits 0. Even if the database is not there, or user isn't created yet & so on.
migration container now starts since db is presumed healthy (meanwhile db is now creating extensions, creating database, granting access to user etc).
migration container exits 1, as db is yet to configure itself with the user/pass/db, so migration fails.
backend container never starts as migration exited non-zero.

If the user performs docker compose up -d again, the containers would rerun thus now fixing this issue (since db's initialization presumably is now complete),

spwoodcock commented 4 months ago

Thanks for clarifying! I understand the issue now.

I'm not sure why the db and user would not exist yet, before the first healthcheck runs.

But if this is the case, and tweaking the interval also doesn't help, then like you say a custom entrypoint is possible.

The backend container could have a migrate-entrypoint.sh, which includes a check on the db first before running the migration code.

FMTM has something similar with bundled entrypoints in the backend container than can be selected based on the context