Add devcontainer config #117
No reviewers
Labels
No labels
Closed As
Duplicate
Closed As
Fixed
Closed As
Invalid
easyfix
enhancement
ai-review-please
Backlog Status
Needs Review
Backlog Status
Ready
chore
documentation
points
01
points
02
points
03
points
05
points
08
points
13
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Sprint Status
Blocked
Sprint Status
Done
Sprint Status
In Progress
Sprint Status
Review
Sprint Status
To Do
Technical Debt
Work Item
Bug
Work Item
Epic
Work Item
Spike
Work Item
Task
Work Item
User Story
No milestone
No project
4 participants
Notifications
Due date
No due date set.
Blocks
#116 Add DevContainer configuration
quality/testdays-web
Reference
quality/testdays-web!117
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/116-add-devcontainer-config"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes #116
Assisted-by: Claude Code
WIP: Add devcontainer configto Add devcontainer configAI Code Review
📋 MR Summary
This MR adds a complete DevContainer setup to support containerized local development via Docker Compose or Podman.
.devcontainerconfiguration, including a custom Dockerfile and Docker Compose setup linking the app to a PostgreSQL database.postCreateCommand.sh) to handle dependencies, settings, database readiness checks, and mock data loading.Makefileto detect when it is running inside the DevContainer, skipping duplicate Podman deployment and teardown commands..gitignoreanddocs/DEVELOPER.mdto reflect DevContainer usage guidelines.Detailed Code Review
The implementation is robust and thoughtfully designed, especially with considerations for Fedora/Podman environments (e.g., using
zSELinux volume flags, handling rootless Podman UID mapping, and referencing specificPOSTGRESQL_*vars for the Fedora PostgreSQL image). The changes seamlessly integrate with the existingMakefileworkflows while correctly abstracting the database layer when developing natively vs. inside the container. No bugs or security issues were found.✅ Summary
postCreateCommand.sh, modifyingconf/settings.pydirectly 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 thedbhostname from their host machine. Using environment variable overrides (e.g.,SQLALCHEMY_DATABASE_URI) indocker-compose.ymlinstead of modifyingsettings.pycould 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.
Add devcontainer configto WIP: Add devcontainer config@ -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)er. doesn't this just tell you whether we're in any container?
Yes. It was good enough for expected use inside toolbox or devcontainer but I replaced it anyway with more explicit check.
@ -6,0 +20,4 @@### PyCharmPyCharm supports Dev Containers natively (requires Docker; Podman support isunder development).but the earlier comments imply this only works / is only tested with Podman?
The info was not accurate, docker is not required. Updated.
@ -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 0this is saying that within the container everything must act as 'root' because of rootless podman, right? it took me a while to grok.
Yes, that is correct - root inside container, the same way it is being run in OpenShift. It is a bit confusing for sure.
few little nits and queries.
WIP: Add devcontainer configto Add devcontainer configAI Code Review
📋 MR Summary
This MR adds a devcontainer setup, replaces the
testcontainersdependency with a shared Compose-managed PostgreSQL database, and establishes separate development and testing databases to prevent data corruption..devcontainerconfiguration, using a shared database withdocker-compose.yml.testcontainers[postgres]dependency, updating testing logic to use the shared container.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.
while [ -z "$$DB_CONTAINER" ]; do sleep 1; ...loop increate_dbwill hang infinitely if thedbcontainer fails to start (e.g., due to a port conflict or failed image pull). A timeout or retry limit is required to fail fast.forloop that checks the container ID up to a specific number of retries before exiting with an error.✅ Summary
create_dbtarget 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.
Add devcontainer configto WIP: Add devcontainer configWIP: Add devcontainer configto Add devcontainer configReady for review
Adam is already on top of this, I'm dropping myself from review
A few more notes and queries.
@ -0,0 +18,4 @@ports:- "5050:5050"depends_on:db:So...we define the app container here, and we have a separate
docker-compose.ymlfile 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 testpath?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_appormake testin 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.ymljust 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.@ -0,0 +16,4 @@# First container creation may take longer as PostgreSQL initializes its data directoryecho "Waiting for PostgreSQL..."for i in $(seq 1 60); doif python -c "this is a bit weird, is there a reason we can't just use psql or something? do we not have it in context?
Yes, exactly. This is being run inside app container ubi9/python-312 where
psqlis 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.@ -15,3 +15,4 @@POSTGRESQL_USER: testdays-webPOSTGRESQL_PASSWORD: testdays-webPOSTGRESQL_DATABASE: testdays-webPOSTGRESQL_ADMIN_PASSWORD: adminwhy 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_PASSWORDin this PR)?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.
@ -33,0 +34,4 @@- name: Create test databaserun: |python3.12 -c "import psycopg2again, could we just use psql here?
This is also being run in the context of
ubi9/python-312image with nopsqlby default.I do not mind to install
psqlin there but maybe we could keep the testcontainer exactly the same as the build one and keep using this Python snippet instead.@ -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"Um. IMBW, but I think we still don't automatically clean up after ourselves here, do we? The
remove_dbrule should now stop and remove the db container, since it callspodman 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_testsrule and making thetestrule 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...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.
@ -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)......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 justifdef IS_DEVCONTAINER?Now I look at it, the implementation of
IS_TOOLBOXlooks a little over-complex too, though that's outside the scope of this PR.This is supposedly a robust way to make a variable contain strictly "true" or "false". Replaced with something simpler.
@ -22,2 +28,4 @@COMPOSE_CMD := flatpak-spawn --host podman composeelsePODMAN_CMD := podmanCOMPOSE_CMD := podman composeit's a micro-optimization I guess, but instead of defining
COMPOSE_CMDwe could just change the later line that uses it from@$(COMPOSE_CMD) up -d dbto@$(PODMAN_CMD) compose up -d db?Good idea, implemented.
@ -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); \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?
@ -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_CONTAINERshould 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?
Reworked the whole task using
timeoutinstead.@ -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); \Since
test -zsucceeds 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.@ -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)dbremove_db: $(_STOP_DB_PREREQS) ## Stop and remove PostgreSQL DB container and ALL project volumes (data + test db)why are these called
_STOP_DB_PREREQSif they're used forremove_dbbut notstop_db? Surely they should be_REMOVE_DB_PREREQS? Also...since we makestop_dba no-op on devcontainer anyway, we could probably get away with just not bothering with this, and havingstop_dbbe unconditionally a prereq ofremove_db, knowing that it won't actually do anything if we're in a devcontainer...@ -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"there seems to be some confusion going on between
POSTGRESQL_*andDB_*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_dbrule at all? nothing else calls it AFAICS, and if it "runs automatically on container start" (I think this is a reference tocreate-test-db.shelsewhere in this PR, right?), why would you ever need it? Is there some scenario in which a human might want to run this?Removed the whole task.
@ -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 theI don't understand what this note is for. If you already have a volume, create a database manually?
Wow, thanks for such detailed review @adamwill !
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.
.devcontainer/configuration (Dockerfile, devcontainer.json, docker-compose.yml, postCreateCommand.sh).docker-compose.ymlfor unified database management across dev, test, and devcontainers.testcontainersdependency and replaced with direct database connections.init-scripts/create-test-db.shto automatically provision a dedicated test database on container startup.Makefiletargets to conditionally use Docker Compose for database operations based onIS_DEVCONTAINER..forgejo/workflows/ci.yml), Test Infrastructure (tests/conftest.py,tests/db_container.py), Configuration (conf/settings.py.example,testdays/config.py)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 avoidingpsqlin 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.
-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.DB_URIcontainslocalhostordbbefore executing-dcommands.📄 `Makefile` - Updated Makefile to support both traditional and devcontainer workflows, delegating DB management to docker-compose.
create_dbtarget usestimeout 60 $(PODMAN_CMD) wait --condition=healthy $(DB_CONTAINER). The--conditionflag is standard in Docker but may not be supported in older Podman versions for thewaitcommand, or its syntax might differ (e.g.,podman healthcheck run). Verify compatibility with the target Podman versions.--condition=healthyflag is fully supported in the target environment or fallback to a standard loop check if needed.Questions:
timeoutcommand 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.
try_connect_dbfunction modifiesurl.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.tests/, it is safe, but be cautious about reusing this module elsewhere.✅ Summary
podman wait --condition=healthyworks as expected on the target development environments, as differences between Docker and Podman CLIs can cause failures here.postCreateCommand.shbefore running destructive DB migrations.timeoutcommand 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.
@ -24,0 +28,4 @@_APP_PREREQS :=IS_DB_RUNNING := trueifneq ($(IS_DEVCONTAINER), true)I still don't understand why we're using
ifeq/ifneqwith strings that look like booleans but aren't, instead of just usingifdef/ifndef? Surely that's the simplest choice? Drop theIS_DEVCONTAINER ?= falseabove, and make this (and the others below)ifndef IS_DEVCONTAINER?Guess it is safe to do that in context of this app. Fixed.
@ -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_1Looking at compose spec, I think we can do better here. We can specify
container_name: testdaysweb_dbor 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...
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.
Thanks! Just a couple more things pop out at me.
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).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).
@ -23,1 +14,3 @@PODMAN_CMD := podmanendif# In a devcontainer the DB is managed by compose and settings are configuredThis comment now seems a bit marooned, since we simplified things. Why is it here, above the
DB_CONTAINERdefinition 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_DEVCONTAINERbelow?@adamwill Thank you for all reviews! I appreciate your attention to detail.
d2884306107e6a9186b8