Modify db docker service to use the env file

[del9ra]

Fixes #309

What changes did you make?

• moved the environment variables from docker-compose.yml file to .env.docker-example
• replaced the environment section with env_file: - ./app/.env.docker-example in docker-compose.yml
• change the django settings (settings.py) code to use the database variable names that the db container uses

Why did you make the changes (we will use this info to test)?

• to update the db Docker container to use the same .env file as the web container, so we avoid hard-coding values in the docker-compose.yml file

[fyliu]

Thanks for working on this!

I slack messaged Ethan about the sqlite. I think we need to wait for his response to determine how to name the env variables. But, ultimately, we want to get rid of the docker-compose.yml variables like you did here.

[fyliu]

I see there are other places with the "SQL_ENGINE" type of names. You should change them as well. They're commented out, but they're also examples for non-default cases.

This relates to what I said about whether or not changing the variable names make sense. I think if we're not going to run the backend with sqlite anymore, changing the name makes sense. If we want to continue making the backend work with sqlite, then maybe it doesn't make sense to change the variable names to have the "POSTGRES_" prefix. In that case, you can add the "POSTGRES-*" variables and do something like "SQL_USER=$POSTGRES_USER".

[fyliu]

I think the variable name is actually POSTGRES_DB, from the docker-compose.yml

[del9ra]

1. Almost done
2. Blocker: I changed each SQL affix to POSTGRES under this section: # postgres settings for local development, but I didn’t change anything under # sqlite settings for local development since it’s for SQLite. I only changed the variable names, not the values.
   Do I need to replace the SQL prefix with POSTGRES in the .env.docker file as well?
3. ETA - November 15

[fyliu]

Sorry I wasn't more clear before. I was also not thinking clearly about the scope of this issue. We're just trying to remove the hardcoded lines in the compose file. Since the project had support for sqlite, we should just let it keep doing that and add a workaround to do both. I wrote the details on what to do in the comments.

[fyliu]

Since the compose file only used POSTGRES USER, PASSWORD, and DB, you should just modify those 3 variable names.

I was trying to decide the best way to do this, and thinking what if we removed sqlite support to use postgres variable names only. But that's not the goal of this issue, so we should leave the code in a state where sqlite should still work.

This means that we still need to have the SQL USER, PASSWORD, and DATABASE variables since it wouldn't make sense for the sqlite section to assign to variables like POSTGRES_USER. To work around this, we can add assignments in this format for the 3 changed variables:

SQL_USER=POSTGRES_USER
...

Now we're limiting the changes to the docker section of the file and can leave the rest alone.

Please revert the changes to the local development section. I didn't give you clear enough instructions before for that, and I wasn't thinking right in terms of what this issue should be about.

[fyliu]

Please revert the changes to this file. We don't really want to change the behavior of defaulting to sqlite.

Just a note to clarify how these lines work: Notice the first arguments to these os.environ.get functions correspond to the env variable names in the .env.docker.example file. Those would've needed to change to something like "POSTGRES_USER" if we were to switch everything to the postgres variable names. The 2nd arguments are the default values to return in case those variables are not present.

[fyliu]

I think this is not too obvious to test, but if you copied app/.env.docker-example to app/.env.docker and ran ./scripts/buildrun.sh, it actually doesn't run. I put the reason for that in the comments. Thanks for working through this!

[fyliu]

These changes should actually be done in the "postgres settings for docker" section after the other changes rather than here.

The db container needs to read the POSTGRES_DB, etc. variables and our django settings needs to read the SQL_DATABASE, etc. variables. Both sets of variables need to be present for the containers to have the same database credentials and work with one another.

There's no need to change the "sqlite setting..." section.

[fyliu]

This is right, but it's missing the lines from the other comment below. You should move the POSTGRES_ lines above the SQL_ lines to have a visual separation which keeps the POSTGRES_ lines together and the SQL_ lines together.