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

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

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

spwoodcock
Copy link
Member

@spwoodcock spwoodcock commented Jul 4, 2024

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

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation
  • πŸ§‘β€πŸ’» Refactor
  • βœ… Test
  • πŸ€– Build or CI
  • ❓ Other (please specify)

Related Issue

#6487

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

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

@spwoodcock
Copy link
Member Author

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
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Contributor

Related Issue #6498

@mahesh-naxa
Copy link
Collaborator

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:

  1. db service is started.
  2. migration container is created but not started, since it depends_on db container to be healthy.
  3. 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.
  4. migration container now starts since db is presumed healthy (meanwhile db is now creating extensions, creating database, granting access to user etc).
  5. migration container exits 1, as db is yet to configure itself with the user/pass/db, so migration fails.
  6. 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
Copy link
Member Author

spwoodcock commented Jul 15, 2024

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

@dakotabenjamin
Copy link
Member

@spwoodcock can you rebase this? The backend tests should run properly after.

@spwoodcock
Copy link
Member Author

Done πŸ˜„ Let's see!

Copy link

sonarcloud bot commented Jul 30, 2024

@dakotabenjamin dakotabenjamin merged commit 09153d1 into hotosm:develop Aug 13, 2024
7 checks passed
@spwoodcock spwoodcock deleted the build/dev-bind-mounts branch August 13, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants