fix: resolve DB bootstrap issues on fresh installations#8108
Open
grantfitzsimmons wants to merge 9 commits into
Open
fix: resolve DB bootstrap issues on fresh installations#8108grantfitzsimmons wants to merge 9 commits into
grantfitzsimmons wants to merge 9 commits into
Conversation
Add ALLOW_DB_RESET environment flag and enhance DB setup checks to avoid accidental data loss. .env: add ALLOW_DB_RESET=true with a comment recommending false in production. sp7_db_setup_check.sh: detect presence of a legacy Specify schema by checking for the 'specifyuser' table; if the database exists but lacks the schema, determine whether the DB is empty and proceed, or if non-empty require explicit opt-in via ALLOW_DB_RESET=true to drop and recreate the database. If ALLOW_DB_RESET is not set, the script now aborts with guidance. Also adjust the migration logic to use the legacy-schema check to choose the appropriate base_specify_migration invocation.
Remove the 'restart: unless-stopped' line from the development service in docker-compose.yml
Replace the previous tolerant invocation of sp7_db_setup_check.sh (which used set +e / set -e) with an explicit if-check. If the DB setup script succeeds, continue to run key migration functions; if it fails, print an error and exit with status 1 to avoid continuing startup with an incomplete DB setup.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR hardens the database initialization flow by adding legacy schema detection, making database setup a hard startup requirement, and providing controlled reset behavior via environment configuration. MariaDB bootstrap is simplified by removing redundant user variables that conflict with the official image, and migration execution now branches on schema presence rather than database creation timing. ChangesDatabase setup and legacy schema detection
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
grantfitzsimmons
changed the title
MYSQL_USER and MYSQL_PASSWORD![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
sp7_db_setup_check.sh (1)
460-469: ⚡ Quick winFail fast on
base_specify_migrationso a half-migrated state can't slip through.The conditional swap looks right, but neither
base_specify_migrationnor the subsequentmigratehas its exit status checked. Ifbase_specify_migrationfails, the script still runsmigrateand the script's overall exit status becomesmigrate's — which can mask a failure and produce exactly the partial-migration state issue#8107is trying to eliminate. A small guard makes the contract explicit:♻️ Suggested guard
-if [[ "$LEGACY_SCHEMA_EXISTS" -eq 1 ]]; then - echo "Existing database detected." - ve/bin/python manage.py base_specify_migration --use-override --database=${MIGRATION_DB_ALIAS} -else - echo "Blank or newly created database detected." - ve/bin/python manage.py base_specify_migration --database=${MIGRATION_DB_ALIAS} -fi - -# Run Django migrations -ve/bin/python manage.py migrate --database=${MIGRATION_DB_ALIAS} +if [[ "$LEGACY_SCHEMA_EXISTS" -eq 1 ]]; then + echo "Existing database detected." + if ! ve/bin/python manage.py base_specify_migration --use-override --database="${MIGRATION_DB_ALIAS}"; then + echo "Error: base_specify_migration failed on existing database; aborting before Django migrations." + exit 1 + fi +else + echo "Blank or newly created database detected." + if ! ve/bin/python manage.py base_specify_migration --database="${MIGRATION_DB_ALIAS}"; then + echo "Error: base_specify_migration failed; aborting before Django migrations." + exit 1 + fi +fi + +# Run Django migrations +if ! ve/bin/python manage.py migrate --database="${MIGRATION_DB_ALIAS}"; then + echo "Error: Django migrate failed." + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sp7_db_setup_check.sh` around lines 460 - 469, The script currently runs ve/bin/python manage.py base_specify_migration (conditional on LEGACY_SCHEMA_EXISTS) and then always runs ve/bin/python manage.py migrate without checking return codes; update the flow to fail fast: after each base_specify_migration invocation (both branches that call base_specify_migration with --use-override or without), check its exit status and if non-zero immediately exit the script with that status (so the subsequent migrate is not run on a failed base_specify_migration); apply the same pattern of explicit exit checking around the migrate invocation if you want symmetry, referencing the commands/variables base_specify_migration, migrate, LEGACY_SCHEMA_EXISTS, and MIGRATION_DB_ALIAS to locate the calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env:
- Around line 6-8: The ALLOW_DB_RESET flag currently defaults to true which
enables destructive DB resets; change its default value to false so destructive
reset is opt-in by explicitly setting ALLOW_DB_RESET=true when needed, and
verify any code reading ALLOW_DB_RESET (env key name ALLOW_DB_RESET) treats
absence or "false"/"0" as disabled.
In `@docker-entrypoint.sh`:
- Line 15: The inline comment beside the command "ve/bin/python manage.py
run_key_migration_functions" is inaccurate because the command is already
active; update the comment to reflect that the migration runs on startup (or
remove the misleading "Uncomment if you want..." wording). Locate the line
containing ve/bin/python manage.py run_key_migration_functions and replace the
comment with a correct note such as "Runs key migration functions on startup" or
delete the comment entirely to avoid confusion.
In `@sp7_db_setup_check.sh`:
- Around line 148-150: The DB_TABLE_COUNT fallback of "|| DB_TABLE_COUNT=0" can
mask query failures and cause a populated DB to be treated as empty; change the
mariadb invocation that sets DB_TABLE_COUNT so that any non-zero exit or empty
output triggers an error path and exits (do not silently set DB_TABLE_COUNT=0).
Specifically, run the SELECT COUNT(*) command (the line that calls mariadb with
-sse and assigns DB_TABLE_COUNT) and if the command fails or produces an
empty/non-numeric result, log an explicit error and exit with non-zero status
instead of assigning 0; only allow DB_TABLE_COUNT==0 when the command succeeded
and returned a numeric 0, so the ALLOW_DB_RESET/abort logic that follows behaves
correctly.
- Around line 127-131: The current mariadb query can fail and leave
LEGACY_SCHEMA_EXISTS empty (treated as 0), so change the block that sets
LEGACY_SCHEMA_EXISTS (guarded by DB_EXISTS) to explicitly detect query failures:
run the mariadb command and capture its exit status and output into a variable,
then if the command exit code is non-zero or the output is empty/not an integer,
log an error and exit non‑zero (fail fast) rather than treating it as "no legacy
schema"; reference the existing symbols LEGACY_SCHEMA_EXISTS, DB_EXISTS, and the
mariadb invocation so you replace the direct command substitution with a
two-step capture/check and an explicit process exit on failure.
---
Nitpick comments:
In `@sp7_db_setup_check.sh`:
- Around line 460-469: The script currently runs ve/bin/python manage.py
base_specify_migration (conditional on LEGACY_SCHEMA_EXISTS) and then always
runs ve/bin/python manage.py migrate without checking return codes; update the
flow to fail fast: after each base_specify_migration invocation (both branches
that call base_specify_migration with --use-override or without), check its exit
status and if non-zero immediately exit the script with that status (so the
subsequent migrate is not run on a failed base_specify_migration); apply the
same pattern of explicit exit checking around the migrate invocation if you want
symmetry, referencing the commands/variables base_specify_migration, migrate,
LEGACY_SCHEMA_EXISTS, and MIGRATION_DB_ALIAS to locate the calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 73a2cfe6-6664-4db2-b1ae-c00a5b7f4c3b
📒 Files selected for processing (4)
.envdocker-compose.ymldocker-entrypoint.shsp7_db_setup_check.sh
💤 Files with no reviewable changes (1)
- docker-compose.yml
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
out of an abundance of caution, this detects any query failures explicitly instead of silently defaulting to "no legacy schema".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8107
This PR fixes first-run and recovery behavior for the development Docker setup when MariaDB starts with a database that does not contain a valid Specify schema.
Previously, the startup path could treat a partially initialized database as if it were ready, which led to confusing migration failures and restart loops. The new flow makes the behavior explicit:
Changed
sp7_db_setup_check.shto distinguish between:ALLOW_DB_RESET=trueopt-in for destructive reset of a non-empty, schema-less database. In our development environment, this istruein the default.envfile. If it is not specified, the default isfalse.docker-entrypoint.shso it is obvious that:manage.py base_specify_migrationruns insidesp7_db_setup_check.shmanage.py migrateruns insidesp7_db_setup_check.shmanage.py run_key_migration_functionsis called afterward from the entrypointWith DB Reset
Set this in
.env:Expected
specify7logs:In this mode, the database is recreated and startup continues normally.
Without DB Reset
Set this in
.env:Expected
specify7logs:In this mode, the container exits cleanly and the database is left untouched.
This avoids two failure modes:
Checklist
Testing instructions
docker compose up --build.ALLOW_DB_RESET=truewhen connecting to a dummy database that does not have aspecifyusertable but does contain more than 1 table in it:ALLOW_DB_RESET=true, the stack should drop and recreate the database and continue startupSummary by CodeRabbit
Chores
Bug Fixes