Unify docker compose so local dev needs only an .env, no override yml#1289
Open
vidiecan wants to merge 8 commits into
Open
Unify docker compose so local dev needs only an .env, no override yml#1289vidiecan wants to merge 8 commits into
vidiecan wants to merge 8 commits into
Conversation
Five small changes that make `docker/docker-compose.yml` + `docker-compose-rest.yml`
the same artifact for the pyinfra-driven production deploy AND for a developer
running the stack on their own machine. The prior gap was that compose's defaults
encoded production-only assumptions; local-dev had to override them in a separate
file, which was the friction this fixes.
Defaults preserve every existing production behaviour bit-for-bit (verified with
`docker compose -f ... -f ... config` against an INSTANCE-only env: same subnet,
same proxies trusted range, same host_ip 127.0.0.1, same dspace.ui.url default,
same CORS default that already matched dspace.ui.url implicitly).
Changes:
1. Dockerfile: also copy `docker/dspace-ui.json` to `/app/dspace-ui.json` so the
locally-built image matches Dockerfile.dist's layout. Compose's hardcoded
`pm2-runtime start dspace-ui.json` (no `docker/` prefix) was ENOENT-looping
on locally-built containers. The Dockerfile CMD now matches compose's
entrypoint too.
2. docker-compose.yml: add `extra_hosts: host.docker.internal:host-gateway` to
the FE service. No-op on production (BE is reached via dev-5.pc DNS, not
host.docker.internal). On local dev it lets the FE container's SSR reach a
BE port published on the host machine.
3. docker-compose-rest.yml: parameterise the dspacenet subnet via
`${DSPACE_SUBNET_PREFIX:-172.2${INSTANCE}}.0.0/16`. Default unchanged.
Local dev can move off the crowded 172.2X range with a one-line env override.
`proxies.trusted.ipranges` follows the same prefix automatically.
4. docker-compose-rest.yml: add explicit
`rest__P__cors__P__allowed__D__origins: ${REST_CORS_ALLOWED_ORIGINS:-${UI_URL:-...}}`
to the dspace service env. Default value matches the implicit
`${dspace.ui.url}` fallback that `dspace/config/modules/rest.cfg` already
computes, so production CORS is byte-identical. Local dev can extend with
multiple browser origins (localhost + host.docker.internal + LAN IP) so the
preflight succeeds regardless of which hostname resolves the BE first.
5. New `docker/.env.local.example`: documented starter for local-dev. Sets the
four env vars above (HOST_IP=0.0.0.0, DSPACE_HOST=host.docker.internal,
DSPACE_SUBNET_PREFIX, REST_CORS_ALLOWED_ORIGINS) plus image set and INSTANCE.
Production deploys do not use this file — pyinfra still templates its own
.env from devops/infra/pyinfra/assets/dspace/dspace-envs/v7/.env.j2.
Background: I wrote the diagnosis up in detail when investigating the home-page
flicker (separate PR #1287). The death-by-a-thousand-cuts of unstated assumptions
in the compose files is what made starting the stack locally a slog. This PR
removes the slog without touching the production deploy path.
There was a problem hiding this comment.
Pull request overview
This PR makes the Docker Compose setup usable for both pyinfra-style deployments and local development without a separate override file.
Changes:
- Aligns the dev
Dockerfilewith the dist image layout fordspace-ui.json. - Adds local host gateway resolution for the frontend container.
- Parameterizes backend subnet/CORS settings and adds a local
.envexample.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
Dockerfile |
Copies docker/dspace-ui.json to the runtime path expected by Compose. |
docker/docker-compose.yml |
Adds host.docker.internal mapping for frontend-to-host access. |
docker/docker-compose-rest.yml |
Makes subnet and CORS settings configurable via environment variables. |
docker/.env.local.example |
Documents a starter local development environment file. |
Comments suppressed due to low confidence (1)
docker/.env.local.example:44
- This CORS allow-list is also tied to instance 7 even though the example invites changing
INSTANCE. With any other instance, the browser origin becomeshttp://localhost:400${INSTANCE}and preflight requests will be rejected. Make the origin ports followINSTANCE, or document this line as another required edit when changing the instance.
REST_CORS_ALLOWED_ORIGINS=http://localhost:4007,http://host.docker.internal:4007,http://127.0.0.1:4007
- docker/.env.local.example: make every port follow INSTANCE via nested
${VAR} substitution. Previously hardcoded 4007/8087 silently drifted
from the compose-published ports when a developer changed INSTANCE.
Verified: changing INSTANCE=7 -> 6 now retargets the entire stack
(REST 8086, FE 4006, CORS origins all 400N) with no other edits.
- .gitignore: ignore docker/.env.local so the per-developer copy of the
example doesn't get committed by accident. The .example template stays
committed.
…low-up)
- Derive COMPOSE_PROJECT_NAME from INSTANCE so changing INSTANCE alone really
re-targets the whole stack (drop the hardcoded -p dspace-7 from the usage block).
- DRY REST_URL/UI_URL onto ${DSPACE_HOST}; note that a LAN-IP switch must also be
added to REST_CORS_ALLOWED_ORIGINS.
- HOST_IP=0.0.0.0: spell out the LAN-exposure risk (BE, JVM debug, Postgres, Solr);
native-FE users keep 127.0.0.1.
- Document the Compose-v2 requirement and the top-down (backward-only) env-file
interpolation ordering.
- rest.cors.allowed-origins comment: reframe as a behavior-preserving always-on env
override (value equals rest.cfg's ${dspace.ui.url} default) rather than "byte-identical".
- Fix the stale commented dspace-ui.json volume path (/app/docker -> /app) to match the
post-cp layout.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Single-command BE spin-up via docker-compose-rest.yml + one .env.local, native live-editable FE pointed at it via DSPACE_REST_* env vars, integration/CORS checks, and the gotchas hit while validating (Git-Bash path mangling of /server, Node 18/20 requirement, stale node_modules, stale pgdata volume version mismatch, orphaned ng serve). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replaces the unvalidated first cut. Native FE now verified rendering in a browser against the dockerized BE. Adds the hard-won prerequisites/gotchas: Node 18 (not 24), copy-webpack-plugin ^11 to unbreak `ng serve`, DSPACE_REST_HOST=127.0.0.1 (Node IPv6 localhost trap), and pairing the dtq-dev FE with the dtq-dev-7.5 BE (HAL parser crash on a mismatched BE). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit e84bb29e97a57eb083ab55a3178af70f2a773427)
…rve` webpack/webpack.common.ts resolves the project's pinned copy-webpack-plugin, but @angular-devkit/build-angular@15 emits asset-copy patterns using the `priority` option (copy-webpack-plugin v9+). With the old ^6.4.1 pin, `ng serve` (via @angular-builders/custom-webpack) dies with "Copy Plugin ... unknown property 'priority'". ^11.0.0 matches build-angular@15's own dependency and dedupes to a single resolution; the project's copy options (from/to/force/transform/noErrorOnMissing) stay v11-compatible. CI didn't catch this because it runs build:prod, not ng serve. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 3a2264a3367f16124609fa65cd0f3687be6dd6c0)
Validated the native FE rendering the LINDAT/CLARIAH-CZ home with demo content. Key corrections vs the first cut: - Match the BE to the FE version (7.6.5). dtq-dev-7.5 is DSpace 7.5 -> browse-definitions parse error in the console. - The earlier "Maximum call stack" was the localhost/127.0.0.1 host mismatch (HAL self-links resolved under two hostnames), not a BE-version issue; keep REST_HOST and REST_URL on 127.0.0.1. - Add Option A (upstream 7.6.5 + db.entities.yml + index-discovery -b) for sample content, with the caveat it's upstream (not CLARIN) data; Option B for CLARIN fidelity via a dataquest dump. - Note the MSYS path-mangling also bites `docker exec /dspace/...` (use MSYS_NO_PATHCONV=1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit e156c97623327e2aa7bd7aec65ecb6d1ad01ffdc)
📝 WalkthroughWalkthroughThis PR establishes a parameterized local Docker development environment for frontend/backend integration testing. It introduces environment variable templates, updates Docker Compose configuration for networking and CORS, modifies the application startup process, upgrades build tooling for Angular compatibility, documents the complete workflow including troubleshooting, and adds version control safety for local files. ChangesLocal Docker Development Environment Setup
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docker/docker-compose-rest.yml (1)
70-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefault backend image still points to 7.5 (
dtq-dev-7.5).If
DSPACE_REST_IMAGEis unset, this reintroduces FE/BE version skew and the browse-definition failures this PR is trying to eliminate.Proposed fix
- image: ${DSPACE_REST_IMAGE:-dataquest/dspace:dtq-dev-7.5} + image: ${DSPACE_REST_IMAGE:-dataquest/dspace:dspace-7_x}As per coding guidelines: "
**/docker-compose*.yml: Match the backend DSpace version to the frontend version (FE reports 7.6.5) ... not 7.5 which causes browse definition errors."🤖 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 `@docker/docker-compose-rest.yml` at line 70, The docker-compose default image is still set to the 7.5 tag causing FE/BE version skew; update the default DSPACE_REST_IMAGE value from dataquest/dspace:dtq-dev-7.5 to the frontend-aligned tag (dtq-dev-7.6.5) in the docker-compose REST service definition (the line setting image: ${DSPACE_REST_IMAGE:-dataquest/dspace:dtq-dev-7.5}) so that when DSPACE_REST_IMAGE is unset the backend defaults to the FE-reported 7.6.5 version.package.json (1)
1-212:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd Node 18 enforcement to
package.json(engines)
package.jsonhas noenginesconstraint, and the repo has no.nvmrc/.node-version/Volta pin—so Node 20+/24 can be used accidentally. Add anengines.nodeconstraint to lock the intended runtime.Proposed fix
{ "name": "dspace-angular", "version": "7.6.5", + "engines": { + "node": "18.x" + }, "scripts": {🤖 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 `@package.json` around lines 1 - 212, The package.json is missing an engines.node constraint; add an "engines" object to package.json with a Node 18 enforcement (for example "node": "18.x" or "node": ">=18 <19") so CI/dev tools and package managers know the intended runtime; update package.json at the top-level (near "name"/"version"/"scripts") to include the engines.node constraint and commit the change.
🤖 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 `@AGENTS.md`:
- Around line 31-33: The AGENTS.md entry about "The `ng serve` fix" is stale: it
claims the branch pins copy-webpack-plugin@^6.4.1 and instructs bumping to
^11.0.0, but package.json in this PR already uses ^11.0.0; update the text in
the "The `ng serve` fix" paragraph (and the similar block at lines 153-157) to
reflect that the copy-webpack-plugin dependency has already been updated to
^11.0.0 (mention PR `#1289` or remove the patch-step instruction) and remove any
guidance to run yarn install for that change so readers aren’t directed to an
unnecessary patch step. Ensure you reference "copy-webpack-plugin" and
"package.json" in the updated wording.
---
Outside diff comments:
In `@docker/docker-compose-rest.yml`:
- Line 70: The docker-compose default image is still set to the 7.5 tag causing
FE/BE version skew; update the default DSPACE_REST_IMAGE value from
dataquest/dspace:dtq-dev-7.5 to the frontend-aligned tag (dtq-dev-7.6.5) in the
docker-compose REST service definition (the line setting image:
${DSPACE_REST_IMAGE:-dataquest/dspace:dtq-dev-7.5}) so that when
DSPACE_REST_IMAGE is unset the backend defaults to the FE-reported 7.6.5
version.
In `@package.json`:
- Around line 1-212: The package.json is missing an engines.node constraint; add
an "engines" object to package.json with a Node 18 enforcement (for example
"node": "18.x" or "node": ">=18 <19") so CI/dev tools and package managers know
the intended runtime; update package.json at the top-level (near
"name"/"version"/"scripts") to include the engines.node constraint and commit
the change.
🪄 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
Run ID: ff9b421a-a314-4b89-b797-46057430aa6b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.gitignoreAGENTS.mdDockerfiledocker/.env.local.exampledocker/docker-compose-rest.ymldocker/docker-compose.ymlpackage.json
Comment on lines
+31
to
+33
| - **The `ng serve` fix** (Gotcha #2): the branch pins `copy-webpack-plugin@^6.4.1`, which | ||
| breaks `ng serve`. Bump it to `^11.0.0` in `package.json` and `yarn install`. (This is now | ||
| committed on PR #1289.) |
There was a problem hiding this comment.
copy-webpack-plugin gotcha text is stale relative to this PR state.
These sections still say the branch pins ^6.4.1, but package.json in this PR already sets ^11.0.0. Please update wording to avoid sending readers on an unnecessary patch step.
Also applies to: 153-157
🤖 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 `@AGENTS.md` around lines 31 - 33, The AGENTS.md entry about "The `ng serve`
fix" is stale: it claims the branch pins copy-webpack-plugin@^6.4.1 and
instructs bumping to ^11.0.0, but package.json in this PR already uses ^11.0.0;
update the text in the "The `ng serve` fix" paragraph (and the similar block at
lines 153-157) to reflect that the copy-webpack-plugin dependency has already
been updated to ^11.0.0 (mention PR `#1289` or remove the patch-step instruction)
and remove any guidance to run yarn install for that change so readers aren’t
directed to an unnecessary patch step. Ensure you reference
"copy-webpack-plugin" and "package.json" in the updated wording.
…v:local + .nvmrc) Makes "debuggable/changeable FE against a real backend" two commands: build-scripts/run/dev.backend.sh # DSpace 7.6.5 + demo content at 127.0.0.1:8087, reindexed yarn start:dev:local # ng serve, live-reload on :4000, REST_* env baked in Removes the traps that made this painful: the script pins the 7.6.5 images (FE-matched), uses 127.0.0.1 (Node sends localhost->IPv6; BE is IPv4-only), sets CORS, layers db.entities.yml for sample data, and runs index-discovery with MSYS_NO_PATHCONV. .nvmrc pins Node 18 (the Angular 15 toolchain breaks on newer Node; pairs with the copy-webpack-plugin fix). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit c686c3ce0545387763a02b85212892cbaa2c6b16)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@build-scripts/run/dev.backend.sh`:
- Around line 54-59: The readiness loop calling curl against "$REST" can block
each iteration because it lacks HTTP timeouts; update the curl invocations
inside the for loop and the final status check to include sensible timeouts
(e.g., --connect-timeout 2 and --max-time 5) so each probe is bounded; modify
the two places where curl is called (the conditional in the for loop and the
final if check) to pass these flags while preserving the existing flags (-s -o
/dev/null -w '%{http_code}' and the 2>/dev/null redirection) so the loop still
breaks on HTTP 200 but won't hang indefinitely.
- Around line 63-67: The script currently swallows failures from the
MSYS_NO_PATHCONV=1 docker exec "dspace${INSTANCE}" /dspace/bin/dspace
index-discovery -b command by using "|| echo ...", so change it to fail-fast:
run the same command (the MSYS_NO_PATHCONV docker exec line) and if it exits
non-zero print a clear error like "Indexing Solr discovery failed" and exit 1
(or simply let the non-zero exit propagate), removing the fallback echo; do not
re-add a DB migration step since database migration is already handled
elsewhere.
- Line 27: Remove the hard-coded export of DSPACE_REST_NAMESPACE (the line
exporting DSPACE_REST_NAMESPACE=/server) so the script falls back to the default
config value, and update the docker exec invocation that runs index-discovery -b
to be prefixed with MSYS_NO_PATHCONV=1 and MSYS2_ARG_CONV_EXCL='*' (preserving
the retry hint) to avoid Git-Bash path conversion issues; modify the command in
the retry loop / docker exec block that calls index-discovery -b accordingly.
🪄 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
Run ID: 2d177107-acfd-43c4-b032-8cbb08e35320
📒 Files selected for processing (4)
.nvmrcAGENTS.mdbuild-scripts/run/dev.backend.shpackage.json
✅ Files skipped from review due to trivial changes (2)
- .nvmrc
- AGENTS.md
| export INSTANCE="${INSTANCE:-7}" | ||
| export COMPOSE_PROJECT_NAME="dspace-${INSTANCE}" | ||
| export DSPACE_HOST=127.0.0.1 | ||
| export DSPACE_REST_NAMESPACE=/server |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="build-scripts/run/dev.backend.sh"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "== File: $FILE =="
echo "-- Lines 1-120 --"
nl -ba "$FILE" | sed -n '1,120p'
echo "-- Context around line 27 --"
nl -ba "$FILE" | sed -n '20,40p'
echo "-- Context around docker exec readiness/indexing --"
nl -ba "$FILE" | sed -n '50,90p'Repository: dataquest-dev/dspace-angular
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="build-scripts/run/dev.backend.sh"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "== File: $FILE =="
echo "-- Lines 1-120 --"
awk 'NR>=1 && NR<=120 {printf "%5d\t%s\n", NR, $0}' "$FILE"
echo "-- Context around line 27 --"
awk 'NR>=20 && NR<=40 {printf "%5d\t%s\n", NR, $0}' "$FILE"
echo "-- Context around docker exec readiness/indexing --"
awk 'NR>=55 && NR<=100 {printf "%5d\t%s\n", NR, $0}' "$FILE"Repository: dataquest-dev/dspace-angular
Length of output: 6336
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="build-scripts/run/dev.backend.sh"
echo "== File line count =="
wc -l "$FILE"
echo "== Tail (last 120 lines) =="
awk 'NR> (NR_TOTAL-120) {print}' /dev/null
# can't use NR_TOTAL; do it simpler:
awk 'NR>1000{next} {print}' "$FILE" >/dev/null
# Print from 70 to end (should include any migrate step if present)
echo "== From line 70 to end =="
awk 'NR>=70 {printf "%5d\t%s\n", NR, $0}' "$FILE"Repository: dataquest-dev/dspace-angular
Length of output: 572
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show where DSPACE_REST_NAMESPACE is referenced
rg -n --hidden --no-ignore -S "DSPACE_REST_NAMESPACE" . || true
# Show compose files referenced by the script
ls -la docker || true
ls -la docker/docker-compose-rest.yml docker/db.entities.yml 2>/dev/null || true
# Search for env pass-through / variable substitution in those compose files
for f in docker/docker-compose-rest.yml docker/db.entities.yml; do
if [ -f "$f" ]; then
echo "== $f =="
rg -n "DSPACE_REST_NAMESPACE|REST_NAMESPACE|namespace" "$f" || true
echo "-- file snippet around any matches (if any) --"
rg -n -C 2 "DSPACE_REST_NAMESPACE" "$f" || true
fi
done
# Search for MSYS2_ARG_CONV_EXCL usage in repo
rg -n --hidden --no-ignore -S "MSYS2_ARG_CONV_EXCL" . || trueRepository: dataquest-dev/dspace-angular
Length of output: 3512
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Script: find all docker exec occurrences
echo "== docker exec usages in dev.backend.sh =="
rg -n "docker exec|MSYS_NO_PATHCONV|MSYS2_ARG_CONV_EXCL|index-discovery|database migrate" build-scripts/run/dev.backend.sh || true
# Compose files: inspect migrate/index-discovery sections
echo "== docker/docker-compose-rest.yml migrate occurrences =="
rg -n "database migrate|index-discovery|DSPACE_REST_NAMESPACE" docker/docker-compose-rest.yml
echo "== docker/docker-compose-rest.yml context around migrate =="
# print around the migrate force line we saw earlier
awk 'NR>=110 && NR<=150 {printf "%5d\t%s\n", NR, $0}' docker/docker-compose-rest.yml
echo "== docker/db.entities.yml context (first 220 lines) =="
awk 'NR<=220 {printf "%5d\t%s\n", NR, $0}' docker/db.entities.yml
echo "== repo: migrate ignored occurrences =="
rg -n "database migrate ignored|migrate ignored" . || true
echo "== repo: index-discovery occurrences =="
rg -n "index-discovery" build-scripts/run/dev.backend.sh docker/docker-compose-rest.yml docker/docker-compose.yml docker/db.entities.yml || trueRepository: dataquest-dev/dspace-angular
Length of output: 6893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="AGENTS.md"
awk 'NR>=90 && NR<=210 {printf "%5d\t%s\n", NR, $0}' "$FILE"Repository: dataquest-dev/dspace-angular
Length of output: 7612
Harden Git-Bash compatibility for path conversion.
- Line 27 should not export
DSPACE_REST_NAMESPACE=/serverfrom the shell (Git Bash mangles leading-slash paths); rely on the config/default/serverinstead. - Line 65 should prefix the
docker exec ... index-discovery -bwith bothMSYS_NO_PATHCONV=1andMSYS2_ARG_CONV_EXCL='*'(including the retry hint).
Proposed fix
-export DSPACE_REST_NAMESPACE=/server
+# Do not export DSPACE_REST_NAMESPACE here; rely on config default '/server'
@@
-MSYS_NO_PATHCONV=1 docker exec "dspace${INSTANCE}" /dspace/bin/dspace index-discovery -b \
- || echo " (reindex failed — rerun: MSYS_NO_PATHCONV=1 docker exec dspace${INSTANCE} /dspace/bin/dspace index-discovery -b)"
+MSYS_NO_PATHCONV=1 MSYS2_ARG_CONV_EXCL='*' docker exec "dspace${INSTANCE}" /dspace/bin/dspace index-discovery -b \
+ || echo " (reindex failed — rerun: MSYS_NO_PATHCONV=1 MSYS2_ARG_CONV_EXCL='*' docker exec dspace${INSTANCE} /dspace/bin/dspace index-discovery -b)"🤖 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 `@build-scripts/run/dev.backend.sh` at line 27, Remove the hard-coded export of
DSPACE_REST_NAMESPACE (the line exporting DSPACE_REST_NAMESPACE=/server) so the
script falls back to the default config value, and update the docker exec
invocation that runs index-discovery -b to be prefixed with MSYS_NO_PATHCONV=1
and MSYS2_ARG_CONV_EXCL='*' (preserving the retry hint) to avoid Git-Bash path
conversion issues; modify the command in the retry loop / docker exec block that
calls index-discovery -b accordingly.
Comment on lines
+54
to
+59
| for _ in $(seq 1 90); do | ||
| [ "$(curl -s -o /dev/null -w '%{http_code}' "$REST" 2>/dev/null)" = "200" ] && { echo " ready"; break; } | ||
| printf '.'; sleep 5 | ||
| done | ||
| if [ "$(curl -s -o /dev/null -w '%{http_code}' "$REST" 2>/dev/null)" != "200" ]; then | ||
| echo " timed out. Check: ${COMPOSE[*]} logs dspace${INSTANCE}" >&2 |
There was a problem hiding this comment.
Bound each readiness probe with curl timeouts.
The loop is intended to cap wait time, but each curl call can still block too long without --connect-timeout/--max-time.
Proposed fix
-for _ in $(seq 1 90); do
- [ "$(curl -s -o /dev/null -w '%{http_code}' "$REST" 2>/dev/null)" = "200" ] && { echo " ready"; break; }
+for _ in $(seq 1 90); do
+ [ "$(curl --connect-timeout 2 --max-time 3 -s -o /dev/null -w '%{http_code}' "$REST" 2>/dev/null)" = "200" ] && { echo " ready"; break; }
printf '.'; sleep 5
done
-if [ "$(curl -s -o /dev/null -w '%{http_code}' "$REST" 2>/dev/null)" != "200" ]; then
+if [ "$(curl --connect-timeout 2 --max-time 3 -s -o /dev/null -w '%{http_code}' "$REST" 2>/dev/null)" != "200" ]; then🤖 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 `@build-scripts/run/dev.backend.sh` around lines 54 - 59, The readiness loop
calling curl against "$REST" can block each iteration because it lacks HTTP
timeouts; update the curl invocations inside the for loop and the final status
check to include sensible timeouts (e.g., --connect-timeout 2 and --max-time 5)
so each probe is bounded; modify the two places where curl is called (the
conditional in the for loop and the final if check) to pass these flags while
preserving the existing flags (-s -o /dev/null -w '%{http_code}' and the
2>/dev/null redirection) so the loop still breaks on HTTP 200 but won't hang
indefinitely.
Comment on lines
+63
to
+67
| echo ">> indexing Solr discovery (so browse/search/What's New populate)" | ||
| # MSYS_NO_PATHCONV stops Git Bash from rewriting /dspace/... into a Windows path. | ||
| MSYS_NO_PATHCONV=1 docker exec "dspace${INSTANCE}" /dspace/bin/dspace index-discovery -b \ | ||
| || echo " (reindex failed — rerun: MSYS_NO_PATHCONV=1 docker exec dspace${INSTANCE} /dspace/bin/dspace index-discovery -b)" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="build-scripts/run/dev.backend.sh"
echo "== File exists? =="
ls -la "$FILE" || true
echo
echo "== Lines around 63-90 =="
nl -ba "$FILE" | sed -n '55,95p'
echo
echo "== Search for related commands in this file =="
rg -n --hidden --no-ignore-vcs "database migrate|migrate ignored|index-discovery" "$FILE" || true
echo
echo "== Search repo for other occurrences =="
rg -n --hidden --no-ignore-vcs "database migrate ignored|index-discovery -b" . || trueRepository: dataquest-dev/dspace-angular
Length of output: 261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="build-scripts/run/dev.backend.sh"
echo "== Lines 55-95 (with line numbers) =="
awk 'NR>=55 && NR<=95 {printf "%d:%s\n", NR, $0}' "$FILE"
echo
echo "== Commands in this file =="
rg -n --hidden --no-ignore-vcs "database migrate|migrate ignored|index-discovery" "$FILE" || true
echo
echo "== Related commands elsewhere in repo =="
rg -n --hidden --no-ignore-vcs "database migrate ignored|index-discovery -b" . || trueRepository: dataquest-dev/dspace-angular
Length of output: 2490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="build-scripts/run/dev.backend.sh"
echo "== Snippet around earlier setup/load (1-120) =="
awk 'NR>=1 && NR<=120 {printf "%d:%s\n", NR, $0}' "$FILE"
echo
echo "== Search within this script for migrate/index/load/demo =="
rg -n "database migrate ignored|entities|db\.entities\.yml|load.*entities|index-discovery|reindex|demo" "$FILE" || true
echo
echo "== Show docker/db.entities.yml around migrate step =="
DBENT="docker/db.entities.yml"
test -f "$DBENT" && awk 'NR>=1 && NR<=120 {printf "%d:%s\n", NR, $0}' "$DBENT" || echo "missing $DBENT"Repository: dataquest-dev/dspace-angular
Length of output: 7697
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for MSYS2_ARG_CONV_EXCL usage =="
rg -n "MSYS2_ARG_CONV_EXCL" . || true
echo
echo "== Search for index-discovery error handling patterns =="
rg -n "index-discovery -b|reindex failed" build-scripts docker . || true
echo
echo "== Show docker/db.entities.yml entrypoint migrate (for context) =="
rg -n "/dspace/bin/dspace database migrate ignored" docker/db.entities.ymlRepository: dataquest-dev/dspace-angular
Length of output: 1390
Fail fast when index-discovery -b fails (DB migration is already handled).
docker/db.entities.ymlalready runs/dspace/bin/dspace database migrate ignoredin thedspacecontainer entrypoint (after thedspacedb*-loadsqldemo data load), sodev.backend.shdoesn’t need to add a separate migration step before indexing.build-scripts/run/dev.backend.shlines 63-67 swallowdspace index-discovery -berrors via|| echo ...and then still print “Backend ready”, even though Solr browse/search/“What’s New” won’t be populated—exit non-zero (or rerun and fail) when reindexing fails.
🤖 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 `@build-scripts/run/dev.backend.sh` around lines 63 - 67, The script currently
swallows failures from the MSYS_NO_PATHCONV=1 docker exec "dspace${INSTANCE}"
/dspace/bin/dspace index-discovery -b command by using "|| echo ...", so change
it to fail-fast: run the same command (the MSYS_NO_PATHCONV docker exec line)
and if it exits non-zero print a clear error like "Indexing Solr discovery
failed" and exit 1 (or simply let the non-zero exit propagate), removing the
fallback echo; do not re-add a DB migration step since database migration is
already handled elsewhere.
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.
Summary
docker/docker-compose.yml+docker/docker-compose-rest.ymlwork for the pyinfra-driven production deploy AND for a developer running the stack on their own machine. The prior gap was that compose's defaults encoded production-only assumptions, so local-dev had to maintain a separatedocker-compose.local.ymloverride (which I started writing during #1287 and which the user explicitly didn't want).This PR removes the slog while preserving production deploy behavior.
Changes
Dockerfile— also copydocker/dspace-ui.jsonto/app/dspace-ui.jsonto matchDockerfile.dist's layout. Eliminates the ENOENT loop on locally-built containers (compose's hardcodedpm2-runtime start dspace-ui.jsoncouldn't find the file in the dev image).docker/docker-compose.yml— addextra_hosts: host.docker.internal:host-gatewayto the FE service. No-op for production deploys (BE reached via DNS, nothost.docker.internal). Lets the FE container's SSR reach a host-published BE port in local-dev.docker/docker-compose-rest.yml— parameterise the subnet via${DSPACE_SUBNET_PREFIX:-172.2${INSTANCE}}.0.0/16. Default unchanged.proxies.trusted.iprangesfollows the same prefix.docker/docker-compose-rest.yml— addrest__P__cors__P__allowed__D__origins: ${REST_CORS_ALLOWED_ORIGINS:-${UI_URL:-…}}to the BE service env. When unset, the default resolves to${UI_URL}— the same literal asdspace.ui.urlabove, which equals the implicit${dspace.ui.url}default thatdspace/config/modules/rest.cfgalready computes (confirmed:rest.cors.allowed-origins = ${dspace.ui.url}ondataquest-dev/DSpace@dtq-dev). The effective CORS allow-list is unchanged for prod. Note this now sends the value as an explicit env override on every deploy (previously the BE derived it internally); the value matches, but it would shadowrest.cfgif that ever shipped a non-default list.docker/.env.local.example— documented starter for local-dev (HOST_IP=0.0.0.0,DSPACE_HOST=host.docker.internal,DSPACE_SUBNET_PREFIX=10.10${INSTANCE},REST_CORS_ALLOWED_ORIGINS=..., andCOMPOSE_PROJECT_NAME=dspace-${INSTANCE}so changingINSTANCEalone re-targets the whole stack).How pyinfra is unaffected
Pyinfra templates its own .env from
devops/infra/pyinfra/assets/dspace/dspace-envs/v7/.env.j2, which setsDSPACE_HOST=dev-X.pc,UI_URL=http://dev-X.pc:PORT, etc. None of the new env vars (DSPACE_SUBNET_PREFIX,REST_CORS_ALLOWED_ORIGINS,HOST_IP=0.0.0.0) are set there. Compose's:-defaultfallbacks resolve to the exact prior values:.env.local172.2${INSTANCE}.0.0/16(unchanged)10.10${INSTANCE}.0.0/16proxies.trusted.ipranges172.2${INSTANCE}.0(unchanged)10.10${INSTANCE}.0rest.cors.allowed-origins${UI_URL}(value matches${dspace.ui.url}; now emitted explicitly)http://localhost:4007,http://host.docker.internal:4007,http://127.0.0.1:4007host_ipfor published ports127.0.0.1(unchanged)0.0.0.0extra_hostson FEhost.docker.internalpm2-runtime start dspace-ui.json/app/dspace-ui.json)Verified
Ran
docker compose -f docker/docker-compose.yml -f docker/docker-compose-rest.yml configwith:INSTANCE=5set → subnet172.25.0.0/16, host_ip127.0.0.1, ipranges172.25.0, ui.urlhttp://127.0.0.1:4000— byte-identical to the pre-PR render; the only added line is the behavior-equivalent CORS override.docker/.env.local.example→ subnet10.107.0.0/16, host_ip0.0.0.0, CORS includes all 3 local origins, project name followsINSTANCE— local-dev opt-in works.Local-dev was also exercised end-to-end (dockerized BE + a host-published FE on Compose v2.40): the BE came up on a fresh migrated DB and CORS behaved correctly — preflight from the FE origin →
200+ matchingAccess-Control-Allow-Origin, a non-listed origin →403.Review follow-up (in this branch)
docker/.env.local.example: deriveCOMPOSE_PROJECT_NAMEfromINSTANCE(drop the hardcoded-p), DRYREST_URL/UI_URLonto${DSPACE_HOST}, spell out theHOST_IP=0.0.0.0LAN-exposure risk, and document the Compose-v2 / top-down env-file interpolation requirement.dspace-ui.jsonvolume path (/app/docker→/app) to match the post-cp layout.AGENTS.mdwith a tested recipe for a native (live-editable) FE + dockerized BE, plus the gotchas (Git-Bash mangling of/server, Node 18/20 requirement, stalenode_modules, stalepgdatavolume version mismatch, orphanedng serve).Test plan
docker network inspectshow the same subnet).cp docker/.env.local.example docker/.env.local; docker compose --env-file docker/.env.local -f docker/docker-compose.yml -f docker/docker-compose-rest.yml up -d --buildbrings up a healthy stack reachable athttp://localhost:4007/.Summary by CodeRabbit
Documentation
Chores
New Features