Skip to content

Conversation

@paigewilliams
Copy link
Contributor

@paigewilliams paigewilliams commented Oct 25, 2025

asana task

Description

Base setup for using docker during local development. Removes or replaces some of the previous docker related code. Requires a .env file in the /docker directory.

  • build and start the containers with docker compose up --build (add -d to run detached)
  • stop the containers with docker compose down
  • Use docker exec -it to execute commands in the web container, such as running the tests with docker exec -it docker-web-1 python manage.py test

Phase 2

Things to explore once we feel confident in this base setup

  • Github Action to create a docker image and push to a registry, and deploy to AWS on release
  • Explore multi stage builds?
  • Explore having a prod and test (for running locally) images?
  • Dockerfile for nginx
  • Explore using uv
  • Lots of other things I have not thought of yet

@paigewilliams paigewilliams self-assigned this Oct 25, 2025
@paigewilliams paigewilliams requested a review from Copilot October 28, 2025 18:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR modernizes Docker configuration for the TEKDB application, introducing a complete containerization setup with proper orchestration. The changes implement database health checks, proper initialization sequencing, and production-ready configurations.

  • Updates entrypoint script with database readiness checks and fixture loading
  • Modernizes docker-compose with health checks and proper service dependencies
  • Adds comprehensive Dockerfile with GDAL/PostGIS support
  • Includes .dockerignore for optimized build context

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
TEKDB/docker/entrypoint.sh Enhanced startup script with database wait logic, fixture loading, and HTTP socket configuration
TEKDB/docker-compose.yml Restructured compose file with PostGIS 15, health checks, and environment variable configuration
TEKDB/Dockerfile New multi-stage Python 3.11 image with geospatial dependencies and optimized layer caching
TEKDB/.dockerignore Build context exclusions to reduce image size and avoid copying unnecessary files

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

SQL_DATABASE: tekdb
SQL_USER: postgres
SQL_PASSWORD: postgrespw
SECRET_KEY: 'change-me'
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoding SECRET_KEY directly in docker-compose.yml is a security risk, even with a placeholder value. This should be loaded from the .env.dev file or injected as an environment variable at runtime to prevent accidental commits of sensitive values.

Suggested change
SECRET_KEY: 'change-me'

Copilot uses AI. Check for mistakes.
Copy link
Member

@pollardld pollardld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know Docker well enough to review. If it works than approved!

@pollardld pollardld added archive and removed archive labels Oct 30, 2025
Copy link
Member

@rhodges rhodges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like @pollardld , I'm not confident enough in my Docker knowledge to validate the correctness of the code, but in general it looks good, and it's not going to hurt anything by including (since Docker isn't currently part of anyone's deployment). I look forward to playing with this more in the future. Approved

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 22 to 29
echo "Checking for existing users..."
if [ "$(python manage.py shell -c 'from django.contrib.auth import get_user_model; print(get_user_model().objects.count())')" = "0" ]; then
python manage.py loaddata TEKDB/fixtures/default_users_fixture.json
fi
# Load default lookups only if no lookups exist. Use LookupPlanningUnit as the check.
echo "Checking for existing lookups..."
if [ "$(python manage.py shell -c 'from TEKDB.models import LookupPlanningUnit; print(LookupPlanningUnit.objects.count())')" = "0" ]; then
python manage.py loaddata TEKDB/fixtures/default_lookups_fixture.json
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shell commands that execute Python code are vulnerable to command injection if SQL_HOST or other environment variables contain malicious input. While this is a development setup, it's better to avoid using shell -c with Python. Consider creating a small Django management command or Python script file to check for existing users and lookups, which would be more secure and maintainable.

Suggested change
echo "Checking for existing users..."
if [ "$(python manage.py shell -c 'from django.contrib.auth import get_user_model; print(get_user_model().objects.count())')" = "0" ]; then
python manage.py loaddata TEKDB/fixtures/default_users_fixture.json
fi
# Load default lookups only if no lookups exist. Use LookupPlanningUnit as the check.
echo "Checking for existing lookups..."
if [ "$(python manage.py shell -c 'from TEKDB.models import LookupPlanningUnit; print(LookupPlanningUnit.objects.count())')" = "0" ]; then
python manage.py loaddata TEKDB/fixtures/default_lookups_fixture.json
echo "Checking for existing users and lookups..."
counts=$(python manage.py check_counts)
user_count=$(echo "$counts" | grep '^users:' | cut -d':' -f2 | tr -d ' ')
lookup_count=$(echo "$counts" | grep '^lookups:' | cut -d':' -f2 | tr -d ' ')
if [ "$user_count" = "0" ]; then
python manage.py loaddata TEKDB/fixtures/default_users_fixture.json
fi
if [ "$lookup_count" = "0" ]; then
python manage.py loaddata TEKDB/fixtures/default_lookups_fixture.json

Copilot uses AI. Check for mistakes.
Comment on lines 40 to 41
volumes:
- .:/usr/src/app
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Mounting .:/usr/src/app will override the application code that was copied during the Docker build (including installed dependencies in site-packages if they were installed with --editable). This is useful for development hot-reloading, but ensure this is intentional. For production, this volume mount should be removed.

Suggested change
volumes:
- .:/usr/src/app
# volumes:
# - .:/usr/src/app

Copilot uses AI. Check for mistakes.
# Load default users only if no users exist
echo "Checking for existing users..."
if [ "$(python manage.py shell -c 'from django.contrib.auth import get_user_model; print(get_user_model().objects.count())')" = "0" ]; then
python manage.py loaddata TEKDB/fixtures/default_users_fixture.json
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indentation: lines 10, 12-16, and 29 use tabs, while line 24 uses spaces. Shell scripts should consistently use either tabs or spaces (tabs are conventional for shell scripts). Update line 24 to use tabs to match the rest of the file.

Copilot uses AI. Check for mistakes.
ipdb
ipython

# Serve static files in production containers
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whitenoise dependency should be pinned to a specific version or version range for reproducible builds, similar to other dependencies in the file (e.g., django >=4.2.16,<4.3). Consider specifying a version like whitenoise>=6.0.0,<7.0.0.

Copilot uses AI. Check for mistakes.
Comment on lines +36 to +38
# Docker
Dockerfile
docker-compose*.yml
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .dockerignore file excludes Dockerfile and docker-compose*.yml files from the Docker build context. While docker-compose.yml doesn't need to be in the image, the Dockerfile exclusion is unusual and could cause issues if the build process references it. Consider removing the Dockerfile line from .dockerignore.

Copilot uses AI. Check for mistakes.
@paigewilliams paigewilliams marked this pull request as ready for review December 11, 2025 19:29
@paigewilliams
Copy link
Contributor Author

paigewilliams commented Dec 11, 2025

@rhodges @pollardld This is ready for a second review! Would love if y'all could try to run locally and let me know how it goes!

@pollardld pollardld self-requested a review December 16, 2025 22:33
Copy link
Member

@pollardld pollardld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful! I got TEK running locally.

One note for posterity, I had to edit DATABASES in my local_settings.py which now looks like:

DATABASES = {
    "default": {
        "ENGINE": os.environ.get("SQL_ENGINE", "django.contrib.gis.db.backends.postgis"),
        "NAME": os.environ.get("SQL_DATABASE", "tekdb"),
        "USER": os.environ.get("SQL_USER", "postgres"),
        "PASSWORD": os.environ.get("SQL_PASSWORD", ""),
        "HOST": (
            os.environ.get("SQL_HOST")
            or os.environ.get("DB_HOST")
            or "localhost"
        ),
        "PORT": (
            os.environ.get("SQL_PORT")
            or os.environ.get("DB_PORT")
            or "5432"
        ),
    }
}

@paigewilliams
Copy link
Contributor Author

yes, I forgot to add that. Thanks for mentioning. I can update the docs.

uwsgi --http :8000 --master --enable-threads --module TEKDB.wsgi
elif [ "$1" = "dev" ]; then
echo "Starting python development server on :8000"
python manage.py runserver 0.0.0.0:8000
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhodges added some logic to the entrypoint to support dev or prod servers. The Dockerfile uses test by default, but added a docker-compose.prod.yaml to pass the prod arg

@paigewilliams
Copy link
Contributor Author

I added docs on how to run with docker! https://github.com/Ecotrust/TEKDB/wiki/Local-Development-with-Docker

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.

4 participants