Add devcontainer config #117

Merged
jgroman merged 1 commit from feature/116-add-devcontainer-config into develop 2026-06-02 11:00:46 +00:00
Owner
  • Add devcontainer support
  • Remove db testcontainer dependency, db container is shared between local and devcontainer workflow
  • Separate dev and test database: fixes tests messing up dev data

Fixes #116

Assisted-by: Claude Code

- Add devcontainer support - Remove db testcontainer dependency, db container is shared between local and devcontainer workflow - Separate dev and test database: fixes tests messing up dev data Fixes #116 Assisted-by: Claude Code
jgroman self-assigned this 2026-05-26 12:44:04 +00:00
Add devcontainer config
All checks were successful
Run tests / test (pull_request) Successful in 32s
AI Code Review / ai-review (pull_request_target) Successful in 28s
b9573a76b5
Fixes #116

Assisted-by: Claude Code
jgroman changed title from WIP: Add devcontainer config to Add devcontainer config 2026-05-26 12:46:29 +00:00

AI Code Review

📋 MR Summary

This MR adds a complete DevContainer setup to support containerized local development via Docker Compose or Podman.

  • Key Changes:
    • Added a .devcontainer configuration, including a custom Dockerfile and Docker Compose setup linking the app to a PostgreSQL database.
    • Created an automated initialization script (postCreateCommand.sh) to handle dependencies, settings, database readiness checks, and mock data loading.
    • Updated the Makefile to detect when it is running inside the DevContainer, skipping duplicate Podman deployment and teardown commands.
    • Updated .gitignore and docs/DEVELOPER.md to reflect DevContainer usage guidelines.
  • Impact: .devcontainer (Development Environment), Makefile (Build/Execution Scripts), docs/DEVELOPER.md
  • Risk Level: 🟢 Low - These changes strictly affect the local development environment and developer tooling. They do not introduce runtime changes to the production application.

Detailed Code Review

The implementation is robust and thoughtfully designed, especially with considerations for Fedora/Podman environments (e.g., using z SELinux volume flags, handling rootless Podman UID mapping, and referencing specific POSTGRESQL_* vars for the Fedora PostgreSQL image). The changes seamlessly integrate with the existing Makefile workflows while correctly abstracting the database layer when developing natively vs. inside the container. No bugs or security issues were found.

Summary

  • Overall Assessment: The changes look solid and properly encapsulate the development dependencies in a repeatable containerized environment. Ready to merge.
  • Minor Suggestions:
    • In postCreateCommand.sh, modifying conf/settings.py directly alters a host-mounted file. If a developer frequently switches between running locally on the host and inside the DevContainer, they may encounter DNS resolution errors for the db hostname from their host machine. Using environment variable overrides (e.g., SQLALCHEMY_DATABASE_URI) in docker-compose.yml instead of modifying settings.py could prevent this friction.

🤖 AI Code Review | Generated with ai-code-review | Model: gemini-3.1-pro-preview

⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.

## AI Code Review ### 📋 MR Summary This MR adds a complete DevContainer setup to support containerized local development via Docker Compose or Podman. - **Key Changes:** - Added a `.devcontainer` configuration, including a custom Dockerfile and Docker Compose setup linking the app to a PostgreSQL database. - Created an automated initialization script (`postCreateCommand.sh`) to handle dependencies, settings, database readiness checks, and mock data loading. - Updated the `Makefile` to detect when it is running inside the DevContainer, skipping duplicate Podman deployment and teardown commands. - Updated `.gitignore` and `docs/DEVELOPER.md` to reflect DevContainer usage guidelines. - **Impact:** .devcontainer (Development Environment), Makefile (Build/Execution Scripts), docs/DEVELOPER.md - **Risk Level:** 🟢 Low - These changes strictly affect the local development environment and developer tooling. They do not introduce runtime changes to the production application. ### Detailed Code Review The implementation is robust and thoughtfully designed, especially with considerations for Fedora/Podman environments (e.g., using `z` SELinux volume flags, handling rootless Podman UID mapping, and referencing specific `POSTGRESQL_*` vars for the Fedora PostgreSQL image). The changes seamlessly integrate with the existing `Makefile` workflows while correctly abstracting the database layer when developing natively vs. inside the container. No bugs or security issues were found. ### ✅ Summary - **Overall Assessment:** The changes look solid and properly encapsulate the development dependencies in a repeatable containerized environment. Ready to merge. - **Minor Suggestions:** - In `postCreateCommand.sh`, modifying `conf/settings.py` directly alters a host-mounted file. If a developer frequently switches between running locally on the host and inside the DevContainer, they may encounter DNS resolution errors for the `db` hostname from their host machine. Using environment variable overrides (e.g., `SQLALCHEMY_DATABASE_URI`) in `docker-compose.yml` instead of modifying `settings.py` could prevent this friction. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*
jgroman changed title from Add devcontainer config to WIP: Add devcontainer config 2026-05-26 12:47:44 +00:00
Add db port mapping for external db management
All checks were successful
Run tests / test (pull_request) Successful in 31s
33297c86d8
Makefile Outdated
@ -15,2 +15,4 @@
SETTINGS_FILE := ./conf/settings.py
# Check if running inside a devcontainer (DB is managed by compose)
IS_DEVCONTAINER := $(shell (test -f /.dockerenv || test -f /run/.containerenv) && echo true || echo false)
Owner

er. doesn't this just tell you whether we're in any container?

er. doesn't this just tell you whether we're in *any* container?
Author
Owner

Yes. It was good enough for expected use inside toolbox or devcontainer but I replaced it anyway with more explicit check.

Yes. It was good enough for expected use inside toolbox or devcontainer but I replaced it anyway with more explicit check.
jgroman marked this conversation as resolved
@ -6,0 +20,4 @@
### PyCharm
PyCharm supports Dev Containers natively (requires Docker; Podman support is
under development).
Owner

but the earlier comments imply this only works / is only tested with Podman?

but the earlier comments imply this only works / is only tested with Podman?
Author
Owner

The info was not accurate, docker is not required. Updated.

The info was not accurate, docker is not required. Updated.
jgroman marked this conversation as resolved
@ -0,0 +3,4 @@
"dockerComposeFile": "docker-compose.yml",
"service": "app",
"workspaceFolder": "/opt/app-root/src",
// Root is required for rootless Podman: the host UID is mapped to UID 0
Owner

this is saying that within the container everything must act as 'root' because of rootless podman, right? it took me a while to grok.

this is saying that *within* the container everything must act as 'root' because of rootless podman, right? it took me a while to grok.
Author
Owner

Yes, that is correct - root inside container, the same way it is being run in OpenShift. It is a bit confusing for sure.

Yes, that is correct - root inside container, the same way it is being run in OpenShift. It is a bit confusing for sure.
jgroman marked this conversation as resolved
adamwill left a comment

few little nits and queries.

few little nits and queries.
Remove Postgres testcontainer dependency, reuse db devcontainer
Some checks failed
Run tests / test (pull_request) Failing after 27s
357ec9ffd0
Fix test db creation in CI
All checks were successful
Run tests / test (pull_request) Successful in 32s
f3924a46a1
Clarify podman support in PyCharm
All checks were successful
Run tests / test (pull_request) Successful in 31s
AI Code Review / ai-review (pull_request_target) Successful in 1m16s
60b1d602c4
jgroman changed title from WIP: Add devcontainer config to Add devcontainer config 2026-05-27 09:37:16 +00:00

AI Code Review

📋 MR Summary

This MR adds a devcontainer setup, replaces the testcontainers dependency with a shared Compose-managed PostgreSQL database, and establishes separate development and testing databases to prevent data corruption.

  • Key Changes:
    • Added .devcontainer configuration, using a shared database with docker-compose.yml.
    • Removed testcontainers[postgres] dependency, updating testing logic to use the shared container.
    • Implemented automated test database creation via init scripts and CI steps.
    • Updated Makefile to handle both standard and devcontainer execution contexts correctly.
  • Impact: Devcontainer / local environment (.devcontainer/, docker-compose.yml), Tests (tests/conftest.py, tests/db_container.py), CI/CD (.forgejo/workflows/ci.yml), Build tools (Makefile)
  • Risk Level: 🟡 Medium - Modifies core development database lifecycles and testing infrastructure. Logic errors in container orchestration could impact local development and CI pipelines.

Detailed Code Review

The architectural shift away from testcontainers towards a shared Compose-managed database is well-executed and aligns well with standard devcontainer practices. The removal of the testcontainer dependency cleans up the testing setup, and using separate databases for dev and test fixes data pollution safely. A critical logic flaw was identified in the Makefile's container status polling which could result in an infinite hang if the database fails to launch.

📂 File Reviews

📄 `Makefile` - Updated Makefile commands to orchestrate docker-compose logic, handle devcontainer specific paths, and await container health.
  • Major [Logic]: The while [ -z "$$DB_CONTAINER" ]; do sleep 1; ... loop in create_db will hang infinitely if the db container fails to start (e.g., due to a port conflict or failed image pull). A timeout or retry limit is required to fail fast.
    • Suggestion: Replace the infinite while loop with a bounded for loop that checks the container ID up to a specific number of retries before exiting with an error.
	@DB_CONTAINER=""; \
	for i in $$(seq 1 10); do \
	  DB_CONTAINER=$$($(PODMAN_CMD) ps -q --filter label=com.docker.compose.project=testdaysweb --filter label=com.docker.compose.service=db); \
	  if [ -n "$$DB_CONTAINER" ]; then break; fi; \
	  sleep 1; \
	done; \
	if [ -z "$$DB_CONTAINER" ]; then echo "Error: DB container failed to start"; exit 1; fi; \
	$(PODMAN_CMD) wait --condition=healthy $$DB_CONTAINER

Summary

  • Overall Assessment: The devcontainer and database changes are architecturally sound. One logic issue was identified in the Makefile orchestration that should be resolved to prevent local environment hangs.
  • Priority Issues:
    • Fix infinite wait loop in Makefile create_db target when container fails to start.

🤖 AI Code Review | Generated with ai-code-review | Model: gemini-3.1-pro-preview

⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.

## AI Code Review ### 📋 MR Summary This MR adds a devcontainer setup, replaces the `testcontainers` dependency with a shared Compose-managed PostgreSQL database, and establishes separate development and testing databases to prevent data corruption. - **Key Changes:** - Added `.devcontainer` configuration, using a shared database with `docker-compose.yml`. - Removed `testcontainers[postgres]` dependency, updating testing logic to use the shared container. - Implemented automated test database creation via init scripts and CI steps. - Updated Makefile to handle both standard and devcontainer execution contexts correctly. - **Impact:** Devcontainer / local environment (.devcontainer/, docker-compose.yml), Tests (tests/conftest.py, tests/db_container.py), CI/CD (.forgejo/workflows/ci.yml), Build tools (Makefile) - **Risk Level:** 🟡 Medium - Modifies core development database lifecycles and testing infrastructure. Logic errors in container orchestration could impact local development and CI pipelines. ### Detailed Code Review The architectural shift away from testcontainers towards a shared Compose-managed database is well-executed and aligns well with standard devcontainer practices. The removal of the testcontainer dependency cleans up the testing setup, and using separate databases for dev and test fixes data pollution safely. A critical logic flaw was identified in the Makefile's container status polling which could result in an infinite hang if the database fails to launch. #### 📂 File Reviews <details> <summary><strong>📄 `Makefile`</strong> - Updated Makefile commands to orchestrate docker-compose logic, handle devcontainer specific paths, and await container health.</summary> - **Major** [Logic]: The `while [ -z "$$DB_CONTAINER" ]; do sleep 1; ...` loop in `create_db` will hang infinitely if the `db` container fails to start (e.g., due to a port conflict or failed image pull). A timeout or retry limit is required to fail fast. - *Suggestion:* Replace the infinite while loop with a bounded `for` loop that checks the container ID up to a specific number of retries before exiting with an error. ```` @DB_CONTAINER=""; \ for i in $$(seq 1 10); do \ DB_CONTAINER=$$($(PODMAN_CMD) ps -q --filter label=com.docker.compose.project=testdaysweb --filter label=com.docker.compose.service=db); \ if [ -n "$$DB_CONTAINER" ]; then break; fi; \ sleep 1; \ done; \ if [ -z "$$DB_CONTAINER" ]; then echo "Error: DB container failed to start"; exit 1; fi; \ $(PODMAN_CMD) wait --condition=healthy $$DB_CONTAINER ```` </details> ### ✅ Summary - **Overall Assessment:** The devcontainer and database changes are architecturally sound. One logic issue was identified in the Makefile orchestration that should be resolved to prevent local environment hangs. - **Priority Issues:** - Fix infinite wait loop in Makefile `create_db` target when container fails to start. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*
jgroman changed title from Add devcontainer config to WIP: Add devcontainer config 2026-05-27 09:39:08 +00:00
Fix AI review finding: potential infinite loop waiting for db creation
All checks were successful
Run tests / test (pull_request) Successful in 31s
710429d6cf
jgroman changed title from WIP: Add devcontainer config to Add devcontainer config 2026-05-28 09:38:57 +00:00
Author
Owner

Ready for review

Ready for review
Owner

Adam is already on top of this, I'm dropping myself from review

Adam is already on top of this, I'm dropping myself from review
adamwill requested changes 2026-05-28 18:19:49 +00:00
Dismissed
adamwill left a comment

A few more notes and queries.

A few more notes and queries.
@ -0,0 +18,4 @@
ports:
- "5050:5050"
depends_on:
db:
Owner

So...we define the app container here, and we have a separate docker-compose.yml file at the root level which defines the db container, if I'm following things right?

That feels weird. Is this just...how devcontainers does things? Why? Why can't we just have a single file that defines both? Or is this split so that we can use just the db container on the make test path?

So...we define the app container here, and we have a separate `docker-compose.yml` file at the root level which defines the db container, if I'm following things right? That feels weird. Is this just...how devcontainers does things? Why? Why can't we just have a single file that defines both? Or is this split so that we can use just the db container on the `make test` path?
Author
Owner

This was split to stay compatible with "old way" of developing and testing testdays-web. The "old way" would expect you to create python venv and then run make run_app or make test in there. This would in turn create either unmanaged db container for development or python managed testcontainer for testing. It was quite convoluted.
This root docker-compose.yml just creates db container which can be either used for venv development and testing or can be imported by devcontainer compose and reused there as well. When used by devcontainer the db container becomes managed as well.

This was split to stay compatible with "old way" of developing and testing testdays-web. The "old way" would expect you to create python venv and then run `make run_app` or `make test` in there. This would in turn create either unmanaged db container for development or python managed testcontainer for testing. It was quite convoluted. This root `docker-compose.yml` just creates db container which can be either used for venv development and testing or can be imported by devcontainer compose and reused there as well. When used by devcontainer the db container becomes managed as well.
jgroman marked this conversation as resolved
@ -0,0 +16,4 @@
# First container creation may take longer as PostgreSQL initializes its data directory
echo "Waiting for PostgreSQL..."
for i in $(seq 1 60); do
if python -c "
Owner

this is a bit weird, is there a reason we can't just use psql or something? do we not have it in context?

this is a bit weird, is there a reason we can't just use psql or something? do we not have it in context?
Author
Owner

Yes, exactly. This is being run inside app container ubi9/python-312 where psql is not available. So we can either install psql and then run some psql command anyway or we can just use python which is already in there.

Yes, exactly. This is being run inside app container ubi9/python-312 where `psql` is not available. So we can either install psql and then run some psql command anyway or we can just use python which is already in there.
jgroman marked this conversation as resolved
@ -15,3 +15,4 @@
POSTGRESQL_USER: testdays-web
POSTGRESQL_PASSWORD: testdays-web
POSTGRESQL_DATABASE: testdays-web
POSTGRESQL_ADMIN_PASSWORD: admin
Owner

why do we need to define this when we did not before, and it does not appear to be used elsewhere (this is the only occurrence of the string POSTGRESQL_ADMIN_PASSWORD in this PR)?

why do we need to define this when we did not before, and it does not appear to be used elsewhere (this is the only occurrence of the string `POSTGRESQL_ADMIN_PASSWORD` in this PR)?
Author
Owner

This is actually used to define Postgres admin password for db service container created from https://quay.io/repository/fedora/postgresql-16?tab=info. We need this password defined so that we can use it at line 39. Default user cannot do CREATEDB so we need admin for that.

This is actually used to define Postgres admin password for db service container created from <https://quay.io/repository/fedora/postgresql-16?tab=info>. We need this password defined so that we can use it at line 39. Default user cannot do CREATEDB so we need admin for that.
jgroman marked this conversation as resolved
@ -33,0 +34,4 @@
- name: Create test database
run: |
python3.12 -c "
import psycopg2
Owner

again, could we just use psql here?

again, could we just use psql here?
Author
Owner

This is also being run in the context of ubi9/python-312 image with no psql by default.
I do not mind to install psql in there but maybe we could keep the testcontainer exactly the same as the build one and keep using this Python snippet instead.

This is also being run in the context of `ubi9/python-312` image with no `psql` by default. I do not mind to install `psql` in there but maybe we could keep the testcontainer exactly the same as the build one and keep using this Python snippet instead.
jgroman marked this conversation as resolved
Makefile Outdated
@ -101,3 +124,3 @@
test: start_db ## Run pytest tests. Add optional pytest arguments separated by '--'
@echo "Note: You may add optional pytest arguments after '--' separator like this: 'make test -- -v'."
@echo "Executing: 'RUNMODE=test pytest $(CLI_ARGS)'"
@echo "Note: make sure to remove db test container by running 'make clean-test' after you finish testing"
Owner

Um. IMBW, but I think we still don't automatically clean up after ourselves here, do we? The remove_db rule should now stop and remove the db container, since it calls podman compose down, but AFAICS nothing automatically runs that rule on this path. So...should we not keep this message but change it to say ...running 'make remove_db' after...?

Alternatively...we could do something like turning this into a phony run_tests rule and making the test rule just be: test: run_tests remove_db, or something like that? There's the deleted note in a later text file which claims we left the container around to save time on repeated test executions, though...so maybe we don't want to clean up after ourselves? But if so, we should keep this note and also the later deleted one, and just update them appropriately...

Um. IMBW, but I think we still don't automatically clean up after ourselves here, do we? The `remove_db` rule should now stop *and* remove the db container, since it calls `podman compose down`, but AFAICS nothing automatically *runs* that rule on this path. So...should we not keep this message but change it to say `...running 'make remove_db' after...`? Alternatively...we could do something like turning this into a phony `run_tests` rule and making the `test` rule just be: `test: run_tests remove_db`, or something like that? There's the deleted note in a later text file which claims we left the container around to save time on repeated test executions, though...so maybe we *don't* want to clean up after ourselves? But if so, we should keep this note and also the later deleted one, and just update them appropriately...
Author
Owner

This was explicitly reqested by Kamil for the venv dev environment before. The rationale being that db testcontainer takes too long (approx 10 seconds) to start so let's just start it once and then stop and remove it manually after we were done with testing.
With devcontainers db container is managed and reused both for dev and testing - there two separate databases now. It will be stopped automatically once you close the project in IDE.
So I guess I should reinstate the note but diplay it only during local (venv) testing.

This was explicitly reqested by Kamil for the venv dev environment before. The rationale being that db testcontainer takes too long (approx 10 seconds) to start so let's just start it once and then stop and remove it manually after we were done with testing. With devcontainers db container is managed and reused both for dev and testing - there two separate databases now. It will be stopped automatically once you close the project in IDE. So I guess I should reinstate the note but diplay it only during local (venv) testing.
jgroman marked this conversation as resolved
Makefile Outdated
@ -17,0 +18,4 @@
# IS_DEVCONTAINER is set explicitly in .devcontainer/docker-compose.yml,
# so this is devcontainer-specific and does not trigger inside other containers
# (e.g. CI runners) that happen to have /.dockerenv or /run/.containerenv.
IS_DEVCONTAINER := $(if $(filter true,$(IS_DEVCONTAINER)),true,false)
Owner

......what? I have no idea whether or how this works or exactly what it's doing, but I feel somehow sure that whatever it is, there must be a simpler way. I...kinda suspect that we could drop this line entirely and change the ifeq ($(IS_DEVCONTAINER), true) lines later to just ifdef IS_DEVCONTAINER?

Now I look at it, the implementation of IS_TOOLBOX looks a little over-complex too, though that's outside the scope of this PR.

......what? I have no idea whether or how this works or exactly what it's doing, but I feel somehow sure that whatever it is, there *must* be a simpler way. I...kinda suspect that we could drop this line entirely and change the `ifeq ($(IS_DEVCONTAINER), true)` lines later to just `ifdef IS_DEVCONTAINER`? Now I look at it, the implementation of `IS_TOOLBOX` looks a little over-complex too, though that's outside the scope of this PR.
Author
Owner

This is supposedly a robust way to make a variable contain strictly "true" or "false". Replaced with something simpler.

This is supposedly a robust way to make a variable contain strictly "true" or "false". Replaced with something simpler.
jgroman marked this conversation as resolved
Makefile Outdated
@ -22,2 +28,4 @@
COMPOSE_CMD := flatpak-spawn --host podman compose
else
PODMAN_CMD := podman
COMPOSE_CMD := podman compose
Owner

it's a micro-optimization I guess, but instead of defining COMPOSE_CMD we could just change the later line that uses it from @$(COMPOSE_CMD) up -d db to @$(PODMAN_CMD) compose up -d db?

it's a micro-optimization I guess, but instead of defining `COMPOSE_CMD` we could just change the later line that uses it from `@$(COMPOSE_CMD) up -d db` to `@$(PODMAN_CMD) compose up -d db`?
Author
Owner

Good idea, implemented.

Good idea, implemented.
jgroman marked this conversation as resolved
Makefile Outdated
@ -49,0 +60,4 @@
@$(COMPOSE_CMD) up -d db
@echo "Waiting for PostgreSQL to be healthy..."
@RETRIES=30; \
DB_CONTAINER=$$($(PODMAN_CMD) ps -q --filter label=com.docker.compose.project=testdaysweb --filter label=com.docker.compose.service=db); \
Owner

this feels overly complex. Is the name not predictable when it's created via compose? If not could we make it so, or is there not a compose command we can use to discover the name?

this feels overly complex. Is the name not predictable when it's created via compose? If not could we make it so, or is there not a compose command we can use to discover the name?
jgroman marked this conversation as resolved
Makefile Outdated
@ -49,0 +66,4 @@
DB_CONTAINER=$$($(PODMAN_CMD) ps -q --filter label=com.docker.compose.project=testdaysweb --filter label=com.docker.compose.service=db); \
done; \
[ -n "$$DB_CONTAINER" ] || { echo "ERROR: DB container did not start in time"; exit 1; }; \
$(PODMAN_CMD) wait --condition=healthy $$DB_CONTAINER
Owner

should we give this an upper limit somehow? Based on the manpage I suspect this will wait forever if the container never becomes healthy for whatever reason. Maybe we want to give up waiting after X seconds / minutes and exit with a helpful message? Or at least be more explicit about recording status - we could print "Waiting for DB container to be healthy..." or something before we do this?

should we give this an upper limit somehow? Based on the manpage I suspect this will wait forever if the container never becomes healthy for whatever reason. Maybe we want to give up waiting after X seconds / minutes and exit with a helpful message? Or at least be more explicit about recording status - we could print "Waiting for DB container to be healthy..." or something before we do this?
Author
Owner

Reworked the whole task using timeout instead.

Reworked the whole task using `timeout` instead.
jgroman marked this conversation as resolved
Makefile Outdated
@ -49,0 +63,4 @@
DB_CONTAINER=$$($(PODMAN_CMD) ps -q --filter label=com.docker.compose.project=testdaysweb --filter label=com.docker.compose.service=db); \
while [ -z "$$DB_CONTAINER" ] && [ "$$RETRIES" -gt 0 ]; do \
sleep 1; RETRIES=$$((RETRIES - 1)); \
DB_CONTAINER=$$($(PODMAN_CMD) ps -q --filter label=com.docker.compose.project=testdaysweb --filter label=com.docker.compose.service=db); \
Owner

Since test -z succeeds for a variable that's not defined at all, I think we could avoid the repetition of the command here and just start the loop immediately? might need a bit of fiddling if we want to avoid a one second sleep before the first execution of the command I guess, but eh.

Since `test -z` succeeds for a variable that's not defined at all, I think we could avoid the repetition of the command here and just start the loop immediately? might need a bit of fiddling if we want to avoid a one second sleep before the *first* execution of the command I guess, but eh.
jgroman marked this conversation as resolved
Makefile Outdated
@ -68,3 +82,1 @@
remove_db: stop_db ## Stop and remove PostgreSQL DB container
@echo "Removing PostgreSQL DB Container '$(DB_DATABASE)db'"
@$(PODMAN_CMD) container rm $(DB_DATABASE)db
remove_db: $(_STOP_DB_PREREQS) ## Stop and remove PostgreSQL DB container and ALL project volumes (data + test db)
Owner

why are these called _STOP_DB_PREREQS if they're used for remove_db but not stop_db? Surely they should be _REMOVE_DB_PREREQS? Also...since we make stop_db a no-op on devcontainer anyway, we could probably get away with just not bothering with this, and having stop_db be unconditionally a prereq of remove_db, knowing that it won't actually do anything if we're in a devcontainer...

why are these called `_STOP_DB_PREREQS` if they're used for `remove_db` but not `stop_db`? Surely they should be `_REMOVE_DB_PREREQS`? Also...since we make `stop_db` a no-op on devcontainer anyway, we could probably get away with just not bothering with this, and having `stop_db` be unconditionally a prereq of `remove_db`, knowing that it won't actually do anything if we're in a devcontainer...
jgroman marked this conversation as resolved
Makefile Outdated
@ -71,0 +90,4 @@
@$(COMPOSE_CMD) exec db bash -c \
"psql -c \"SELECT 1 FROM pg_database WHERE datname = '\$${POSTGRESQL_DATABASE}-test'\" | grep -q 1 \
|| createdb --owner=\"\$${POSTGRESQL_USER}\" \"\$${POSTGRESQL_DATABASE}-test\""
@echo "Test database ready: $(DB_DATABASE)-test"
Owner

there seems to be some confusion going on between POSTGRESQL_* and DB_* variable names here. Do we need both? Which do we have in which contexts? Which are we supposed to be using where? Can you take a look and clean it up?

and while I'm here: what's the point of this create_test_db rule at all? nothing else calls it AFAICS, and if it "runs automatically on container start" (I think this is a reference to create-test-db.sh elsewhere in this PR, right?), why would you ever need it? Is there some scenario in which a human might want to run this?

there seems to be some confusion going on between `POSTGRESQL_*` and `DB_*` variable names here. Do we need both? Which do we have in which contexts? Which are we supposed to be using where? Can you take a look and clean it up? and while I'm here: what's the point of this `create_test_db` rule at all? nothing else calls it AFAICS, and if it "runs automatically on container start" (I think this is a reference to `create-test-db.sh` elsewhere in this PR, right?), why would you ever need it? Is there some scenario in which a human might want to run this?
Author
Owner

Removed the whole task.

Removed the whole task.
jgroman marked this conversation as resolved
@ -99,1 +178,3 @@
```
You can find more details regarding the PostgreSQL image here: <https://quay.io/repository/fedora/postgresql-16>
> **Existing volumes:** If you already have a `testdays-pgdata` volume from before the
Owner

I don't understand what this note is for. If you already have a volume, create a database manually?

I don't understand what this note is for. If you already have a volume, create a database manually?
jgroman marked this conversation as resolved
Author
Owner

Wow, thanks for such detailed review @adamwill !

Wow, thanks for such detailed review @adamwill !
Add AGENTS.md to save tokens
All checks were successful
Run tests / test (pull_request) Successful in 31s
AI Code Review / ai-review (pull_request_target) Successful in 27s
d5d3e128f3
jgroman 2026-05-29 13:16:16 +00:00

AI Code Review

📋 MR Summary

Adds development container support and refactors local database management using Docker Compose instead of testcontainers. Separates development and test databases to prevent test data corruption.

  • Key Changes:
    • Added .devcontainer/ configuration (Dockerfile, devcontainer.json, docker-compose.yml, postCreateCommand.sh).
    • Introduced root docker-compose.yml for unified database management across dev, test, and devcontainers.
    • Removed testcontainers dependency and replaced with direct database connections.
    • Added init-scripts/create-test-db.sh to automatically provision a dedicated test database on container startup.
    • Updated Makefile targets to conditionally use Docker Compose for database operations based on IS_DEVCONTAINER.
  • Impact: DevContainer Configuration, Docker Compose Setup, Makefile / Build Tooling, CI Pipeline (.forgejo/workflows/ci.yml), Test Infrastructure (tests/conftest.py, tests/db_container.py), Configuration (conf/settings.py.example, testdays/config.py)
  • Risk Level: 🟢 Low - Changes are isolated to development, testing, and CI infrastructure without modifying production application code. The approach avoids known regressions and streamlines the developer experience.

Detailed Code Review

The PR successfully establishes a solid DevContainer environment and cleans up the test infrastructure by removing testcontainers. The separation of development and test databases is a critical improvement. The implementation elegantly navigates the constraint of avoiding psql in the devcontainer by leveraging Python scripts for readiness checks. The CI workflow is appropriately updated to handle the new test database creation logic.

📂 File Reviews

📄 `.devcontainer/postCreateCommand.sh` - Initialization script for DevContainer, installing dependencies, configuring settings, and waiting for the database.
  • Minor [Security]: The mock database initialization commands run with -d (drop). Since this is a DevContainer, it's generally safe, but ensure this cannot accidentally target a production database if environment variables are misconfigured.
    • Suggestion: No immediate fix required, but consider adding a safety check verifying that DB_URI contains localhost or db before executing -d commands.
📄 `Makefile` - Updated Makefile to support both traditional and devcontainer workflows, delegating DB management to docker-compose.
  • Major [Bug]: The create_db target uses timeout 60 $(PODMAN_CMD) wait --condition=healthy $(DB_CONTAINER). The --condition flag is standard in Docker but may not be supported in older Podman versions for the wait command, or its syntax might differ (e.g., podman healthcheck run). Verify compatibility with the target Podman versions.
    • Suggestion: Ensure the --condition=healthy flag is fully supported in the target environment or fallback to a standard loop check if needed.

Questions:

  • Is the timeout command guaranteed to be available on all developer host operating systems (e.g., macOS natively without coreutils)?
📄 `tests/db_container.py` - Updated the database connection probing utility to use SQLAlchemy's URL object.
  • Minor [Bug]: The try_connect_db function modifies url.render_as_string(hide_password=False). While useful for local testing, ensure this does not accidentally leak production credentials into logs if the code is ever repurposed.
    • Suggestion: Since this is restricted to tests/, it is safe, but be cautious about reusing this module elsewhere.

Summary

  • Overall Assessment: The changes significantly improve the developer experience and test reliability without introducing critical issues. Minor checks regarding Podman command compatibility are recommended.
  • Priority Issues:
    • Verify that podman wait --condition=healthy works as expected on the target development environments, as differences between Docker and Podman CLIs can cause failures here.
  • Minor Suggestions:
    • Consider adding a safety check in postCreateCommand.sh before running destructive DB migrations.
    • Verify if timeout command is available on macOS (if macOS is a supported dev host), as it is a GNU coreutils command.

🤖 AI Code Review | Generated with ai-code-review | Model: gemini-3.1-pro-preview

⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.

## AI Code Review ### 📋 MR Summary Adds development container support and refactors local database management using Docker Compose instead of testcontainers. Separates development and test databases to prevent test data corruption. - **Key Changes:** - Added `.devcontainer/` configuration (Dockerfile, devcontainer.json, docker-compose.yml, postCreateCommand.sh). - Introduced root `docker-compose.yml` for unified database management across dev, test, and devcontainers. - Removed `testcontainers` dependency and replaced with direct database connections. - Added `init-scripts/create-test-db.sh` to automatically provision a dedicated test database on container startup. - Updated `Makefile` targets to conditionally use Docker Compose for database operations based on `IS_DEVCONTAINER`. - **Impact:** DevContainer Configuration, Docker Compose Setup, Makefile / Build Tooling, CI Pipeline (`.forgejo/workflows/ci.yml`), Test Infrastructure (`tests/conftest.py`, `tests/db_container.py`), Configuration (`conf/settings.py.example`, `testdays/config.py`) - **Risk Level:** 🟢 Low - Changes are isolated to development, testing, and CI infrastructure without modifying production application code. The approach avoids known regressions and streamlines the developer experience. ### Detailed Code Review The PR successfully establishes a solid DevContainer environment and cleans up the test infrastructure by removing `testcontainers`. The separation of development and test databases is a critical improvement. The implementation elegantly navigates the constraint of avoiding `psql` in the devcontainer by leveraging Python scripts for readiness checks. The CI workflow is appropriately updated to handle the new test database creation logic. #### 📂 File Reviews <details> <summary><strong>📄 `.devcontainer/postCreateCommand.sh`</strong> - Initialization script for DevContainer, installing dependencies, configuring settings, and waiting for the database.</summary> - **Minor** [Security]: The mock database initialization commands run with `-d` (drop). Since this is a DevContainer, it's generally safe, but ensure this cannot accidentally target a production database if environment variables are misconfigured. - *Suggestion:* No immediate fix required, but consider adding a safety check verifying that `DB_URI` contains `localhost` or `db` before executing `-d` commands. </details> <details> <summary><strong>📄 `Makefile`</strong> - Updated Makefile to support both traditional and devcontainer workflows, delegating DB management to docker-compose.</summary> - **Major** [Bug]: The `create_db` target uses `timeout 60 $(PODMAN_CMD) wait --condition=healthy $(DB_CONTAINER)`. The `--condition` flag is standard in Docker but may not be supported in older Podman versions for the `wait` command, or its syntax might differ (e.g., `podman healthcheck run`). Verify compatibility with the target Podman versions. - *Suggestion:* Ensure the `--condition=healthy` flag is fully supported in the target environment or fallback to a standard loop check if needed. **Questions:** - ❓ Is the `timeout` command guaranteed to be available on all developer host operating systems (e.g., macOS natively without coreutils)? </details> <details> <summary><strong>📄 `tests/db_container.py`</strong> - Updated the database connection probing utility to use SQLAlchemy's URL object.</summary> - **Minor** [Bug]: The `try_connect_db` function modifies `url.render_as_string(hide_password=False)`. While useful for local testing, ensure this does not accidentally leak production credentials into logs if the code is ever repurposed. - *Suggestion:* Since this is restricted to `tests/`, it is safe, but be cautious about reusing this module elsewhere. </details> ### ✅ Summary - **Overall Assessment:** The changes significantly improve the developer experience and test reliability without introducing critical issues. Minor checks regarding Podman command compatibility are recommended. - **Priority Issues:** - Verify that `podman wait --condition=healthy` works as expected on the target development environments, as differences between Docker and Podman CLIs can cause failures here. - **Minor Suggestions:** - Consider adding a safety check in `postCreateCommand.sh` before running destructive DB migrations. - Verify if `timeout` command is available on macOS (if macOS is a supported dev host), as it is a GNU coreutils command. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*
Makefile Outdated
@ -24,0 +28,4 @@
_APP_PREREQS :=
IS_DB_RUNNING := true
ifneq ($(IS_DEVCONTAINER), true)
Owner

I still don't understand why we're using ifeq / ifneq with strings that look like booleans but aren't, instead of just using ifdef / ifndef? Surely that's the simplest choice? Drop the IS_DEVCONTAINER ?= false above, and make this (and the others below) ifndef IS_DEVCONTAINER?

I still don't understand why we're using `ifeq` / `ifneq` with strings that look like booleans but aren't, instead of just using `ifdef` / `ifndef`? Surely that's the simplest choice? Drop the `IS_DEVCONTAINER ?= false` above, and make this (and the others below) `ifndef IS_DEVCONTAINER`?
Author
Owner

Guess it is safe to do that in context of this app. Fixed.

Guess it is safe to do that in context of this app. Fixed.
jgroman marked this conversation as resolved
Makefile Outdated
@ -24,0 +24,4 @@
# and podman status checks are not executed.
# Container name follows podman-compose convention: <project>_<service>_<index>.
# The project name "testdaysweb" is fixed in docker-compose.yml.
DB_CONTAINER := testdaysweb_db_1
Owner

Looking at compose spec, I think we can do better here. We can specify container_name: testdaysweb_db or whatever, and then we always know what it's called.

This did cause me to wonder what happens if you ever, e.g., have a devcontainer setup running, then try to run the tests from outside the devcontainer. With your previous approach or this one, compose is able in theory to run multiple db containers; with mine it would not be. But actually I don't think that's what happens, right? They're supposed to use the same db container in that case, right? We can't actually have multiple of them running at once anyway since there's a hardcoded port mapping in the definition...

Looking at [compose spec](https://github.com/compose-spec/compose-spec/blob/main/spec.md#container_name), I think we can do better here. We can specify `container_name: testdaysweb_db` or whatever, and then we always know what it's called. This *did* cause me to wonder what happens if you ever, e.g., have a devcontainer setup running, then try to run the tests from outside the devcontainer. With your previous approach or this one, compose is able in theory to run multiple db containers; with mine it would not be. But actually I don't think that's what happens, right? They're supposed to use the *same* db container in that case, right? We can't actually have multiple of them running at once anyway since there's a hardcoded port mapping in the definition...
Author
Owner

This is useful tip! Now that db name is unified I also updated some other code referring to it.

Well, yes, it is not possible to start multiple devcontainer instances due to hardcoded ports anyway and if we wanted to support this usecase we'd have to add some container name detection etc. I am not sure wheter it would be worth the hassle, though. Maybe if the devcontainer was running on some remote shared system, then it would be useful.
With that said, currently it is possible to have the devcontainer setup running and run tests outside. I tried that and it works. But that would mean you are developing in devcontainer environment and testing in your local environment so such testing would be useful only for a quick check if the app works in say newer Python or something like that.

This is useful tip! Now that db name is unified I also updated some other code referring to it. Well, yes, it is not possible to start multiple devcontainer instances due to hardcoded ports anyway and if we wanted to support this usecase we'd have to add some container name detection etc. I am not sure wheter it would be worth the hassle, though. Maybe if the devcontainer was running on some remote shared system, then it would be useful. With that said, currently it is possible to have the devcontainer setup running and run tests outside. I tried that and it works. But that would mean you are developing in devcontainer environment and testing in your local environment so such testing would be useful only for a quick check if the app works in say newer Python or something like that.
jgroman marked this conversation as resolved
adamwill left a comment

Thanks! Just a couple more things pop out at me.

Thanks! Just a couple more things pop out at me.
Fix review findings: simplify IS_DEVCONTAINER eval, explicitly name db container
All checks were successful
Run tests / test (pull_request) Successful in 34s
b0029a33d8
adamwill approved these changes 2026-06-01 16:20:16 +00:00
adamwill left a comment

OK, just a couple of minor notes on comments and I think this is good to go! Please check the notes and adjust if you think they're right, then I think it's ready for merge.

OK, just a couple of minor notes on comments and I think this is good to go! Please check the notes and adjust if you think they're right, then I think it's ready for merge.
@ -0,0 +8,4 @@
environment:
RUNMODE: dev
# Explicit devcontainer marker used by the Makefile to skip DB management
# (DB is managed by compose) and skip settings setup (done by postCreateCommand).
Owner

the "skip settings setup" part here is now obsolete and should be removed (we aren't doing settings setup in the Makefile any more; the variable is only used to skip DB management).

the "skip settings setup" part here is now obsolete and should be removed (we aren't doing settings setup in the Makefile any more; the variable is only used to skip DB management).
jgroman marked this conversation as resolved
Makefile Outdated
@ -23,1 +14,3 @@
PODMAN_CMD := podman
endif
# In a devcontainer the DB is managed by compose and settings are configured
Owner

This comment now seems a bit marooned, since we simplified things. Why is it here, above the DB_CONTAINER definition which has its own single-line comment ("Container name is set explicitly via container_name in docker-compose.yml")?

I'd suggest changing it to "In a devcontainer the DB is managed by compose, so we skip DB lifecycle management actions" and moving it to just before or after the first ifndef IS_DEVCONTAINER below?

This comment now seems a bit marooned, since we simplified things. Why is it here, above the `DB_CONTAINER` definition which has its own single-line comment ("Container name is set explicitly via container_name in docker-compose.yml")? I'd suggest changing it to "In a devcontainer the DB is managed by compose, so we skip DB lifecycle management actions" and moving it to just before or after the first `ifndef IS_DEVCONTAINER` below?
jgroman marked this conversation as resolved
Fix review findings: update comments
All checks were successful
Run tests / test (pull_request) Successful in 31s
d288430610
Author
Owner

@adamwill Thank you for all reviews! I appreciate your attention to detail.

@adamwill Thank you for all reviews! I appreciate your attention to detail.
jgroman force-pushed feature/116-add-devcontainer-config from d288430610
All checks were successful
Run tests / test (pull_request) Successful in 31s
to 7e6a9186b8
All checks were successful
Run tests / test (pull_request) Successful in 32s
2026-06-02 10:59:46 +00:00
Compare
jgroman merged commit 7e6a9186b8 into develop 2026-06-02 11:00:46 +00:00
jgroman deleted branch feature/116-add-devcontainer-config 2026-06-02 11:00:47 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Blocks
Reference
quality/testdays-web!117
No description provided.