Skip to content

Conversation

@tobiasmcnulty
Copy link
Member

@tobiasmcnulty tobiasmcnulty commented Sep 16, 2025

This PR makes several improvements to streamline the local Docker setup process:

  1. Moves the Trac schema/table creation script out of the entry point and into a one-time initialization script, to avoid Postgres errors when it's inadvertently re-run when the web container is restarted.
  2. Moves the management commands that create data out of the Docker entrypoint and into their own reset-local-db Makefile target.
  3. Updates the Docker instructions with the applicable *.djangoproject.localhost:8000 links and ties back to the non-Docker setup.
  4. Runs tests via Docker, to avoid reoccurrences of Docker only install instructions run into database connection error trac does not exist #2193 (or similar)

@tobiasmcnulty tobiasmcnulty force-pushed the docker-cleanup branch 5 times, most recently from df2a86e to e44015b Compare September 16, 2025 02:40
Copy link

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 streamlines the Docker development setup by improving the initialization process and adding comprehensive test execution via Docker. The changes simplify the dev entrypoint script, reorganize database initialization, and provide clearer documentation for Docker-based development workflows.

  • Moved database initialization logic from the dev entrypoint to dedicated initialization scripts
  • Added a new Makefile target for resetting local development data
  • Enhanced GitHub Actions workflow to run tests via Docker Compose

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
docker-entrypoint.dev.sh Simplified by removing database initialization commands that are now handled elsewhere
docker-entrypoint-initdb.d/02_trac_schema.sh New script to handle Trac database schema initialization during container startup
docker-compose.yml Updated volume mounts to use the new initialization directory structure
README.rst Enhanced Docker documentation with clearer step-by-step instructions and additional usage examples
Makefile Added reset-local-db target to consolidate database reset commands
.github/workflows/docker-test-build.yml Added Docker Compose setup and test execution for the tests.txt matrix
.dockerignore New file to exclude unnecessary directories from Docker build context

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@tobiasmcnulty tobiasmcnulty marked this pull request as ready for review September 16, 2025 03:10
@tobiasmcnulty
Copy link
Member Author

Hi, @easherma! Thanks again for your helping improving the Docker setup at the sprints. Would you be up for giving this a review when you have a moment? No rush.

Copy link
Member

@ulgens ulgens left a comment

Choose a reason for hiding this comment

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

I added a small comment about the migrate command call, but other than it looks nice 🌻

@tobiasmcnulty tobiasmcnulty changed the title Streamline Docker setup and run tests via Docker Streamlined and added CI steps for docker compose Sep 24, 2025
@tobiasmcnulty tobiasmcnulty requested a review from a team September 24, 2025 14:23
@SaptakS
Copy link
Contributor

SaptakS commented Oct 2, 2025

@tobiasmcnulty makes sense to me. I think you need to rebase and resolve conflicts though. Once that is done, I think should be ready for approval.

Comment on lines +28 to 35
- if: matrix.req_file != 'tests.txt'
name: Test docker image build (${{ matrix.req_file }})
uses: docker/build-push-action@v6
with:
context: .
push: false
build-args: |
REQ_FILE=requirements/${{ matrix.req_file }}
Copy link
Member

Choose a reason for hiding this comment

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

For production, we just build the image, and then nothing else? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I think just building the image will be a weak test (and burn CI minutes). Since we are building the image, we might also want to check the startup, something like:

- if: matrix.req_file == 'prod.txt'
  name: Test production image starts successfully
  run: |
    docker compose up -d
    sleep 5
    docker compose ps
    docker compose logs
    # Maybe curl a health endpoint

Copy link
Member Author

Choose a reason for hiding this comment

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

@CuriousLearner Thanks for the feedback. Having some tests for the production image sounds like a positive change to me. I think it could be its own PR as its not really related to the local docker compose setup.

@tobiasmcnulty tobiasmcnulty removed the request for review from bmispelon October 8, 2025 18:38
@ulgens ulgens self-requested a review October 9, 2025 15:07
@tobiasmcnulty tobiasmcnulty merged commit 1612897 into main Oct 13, 2025
5 checks passed
@tobiasmcnulty tobiasmcnulty deleted the docker-cleanup branch October 13, 2025 13:47
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.

5 participants