feat(build-tests): Phase 2 — live end-to-end integration tests (#146)#148
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds dry-run logging and fixture-based validation for ChangesBuild-test harness: dry-run + Phase 1/2 integration
Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant test.sh
participant run-all.sh
participant run-scenario.sh
participant demo_build.sh
GitHubActions->>test.sh: run dry-run fixtures
GitHubActions->>run-scenario.sh: run live scenario
run-all.sh->>run-scenario.sh: invoke each live scenario
test.sh->>demo_build.sh: run --dry-run
run-scenario.sh->>demo_build.sh: use fixture action log
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tools/build-tests/integration/run-scenario.sh (1)
39-40: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the container images by digest.
These live goldens are supposed to catch
demo_build.shregressions, but mutable tags can make the suite fail when upstream republishes either image. Preferimage@sha256:...here and keep tag changes explicit.Also applies to: 164-170
🤖 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 `@tools/build-tests/integration/run-scenario.sh` around lines 39 - 40, The container images used in run-scenario.sh are currently pinned only by mutable tags, which can make the golden tests flaky when upstream republishes images. Update the IMAGE assignment and the other image references in the same script to use digest-pinned image@sha256 identifiers instead of tags, and keep any future tag changes explicit by changing the digest reference in one place..github/workflows/build-tests-live.yml (1)
22-29: 🚀 Performance & Scalability | 🔵 TrivialResolve the pre-merge trigger TODO before landing.
This still ships
pull_requestand omits the nightlyschedule, so the repo keeps the 20–45 minute PR cost and never gets the intended post-merge coverage described in the header and PR objective.🤖 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 @.github/workflows/build-tests-live.yml around lines 22 - 29, Update the workflow trigger configuration in the build-tests-live workflow so it no longer runs on pull_request and instead includes the intended nightly schedule trigger. Keep the existing path filters for the relevant build/test files, and make sure the workflow’s trigger block matches the post-merge coverage goal described by the workflow header and objective..shellcheckrc (1)
36-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope these ShellCheck disables to
demo_build.shThese are repo-wide suppressions for bashisms that all come from
demo_build.sh’s stale#!/bin/shshebang. Switch that script to bash, or scope the suppressions there, so the rest of the repo keeps ShellCheck coverage.🤖 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 @.shellcheckrc around lines 36 - 37, The ShellCheck suppressions for SC3043 and SC3050 are currently applied repo-wide in .shellcheckrc, but they only relate to bashisms used by demo_build.sh. Move or scope these disables to demo_build.sh, or update demo_build.sh to use bash so the rest of the repo keeps ShellCheck coverage; use the demo_build.sh script and the .shellcheckrc entries as the places to adjust.demo_build.sh (1)
821-824: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
mkdir -pfor$FILESSERVEDIRto match Line 773 and stay idempotent.Line 773 uses
mkdir -p(unwrapped, by design), but Line 824 uses baremkdir, which errors if$FILESSERVEDIRalready exists or its parent is missing—e.g. on a re-invocation in a running container.-pkeeps the unwrapped-for-dry-run intent while remaining idempotent.♻️ Suggested change
- mkdir $FILESSERVEDIR + mkdir -p $FILESSERVEDIR🤖 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 `@demo_build.sh` around lines 821 - 824, The `$FILESSERVEDIR` setup in the web file directory creation step should be idempotent like the earlier directory creation logic. Update the `mkdir` call used before the `cd $FILESSERVEDIR` flow to use the same `-p` behavior as the other directory setup, so reruns do not fail if the directory already exists or the parent path is missing.
🤖 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 `@demo_build.sh`:
- Around line 56-72: Extend the redaction in _emit_action_log so it also strips
Composer GitHub credentials from live-run output, not just mariadb passwords and
rootpass values. Update the sed-based filtering to catch Authorization headers
and the github-oauth.github.com token form emitted by the curl and composer
config paths, so ACTION_LOG cannot capture $GITHUB_KEY_COMPOSER or similar
secrets during Phase 2 runs.
In `@tools/build-tests/integration/run-scenario.sh`:
- Around line 43-46: The CLI parsing in run-scenario.sh currently lets the
fixture source overwrite the --image value, so --image is not the
highest-precedence override. Update the image assignment flow so the
fixture-provided value is only used as a default when IMAGE was not already set
by the argument parser in the while/case block, preserving the user-supplied
--image for all scenarios including those with INTEGRATION_IMAGE.
In `@tools/build-tests/test.sh`:
- Around line 18-24: Update the fixture path example in the `test.sh` comments
so it matches how `cp -a "$extra_dir/." "$work/"` applies overrides. The
`extra/openemr/version.php` example should include the `web/` prefix to reflect
the actual `"$WEB/openemr/version.php"` location, and the surrounding guidance
for `extra/` marker contents should be adjusted consistently.
---
Nitpick comments:
In @.github/workflows/build-tests-live.yml:
- Around line 22-29: Update the workflow trigger configuration in the
build-tests-live workflow so it no longer runs on pull_request and instead
includes the intended nightly schedule trigger. Keep the existing path filters
for the relevant build/test files, and make sure the workflow’s trigger block
matches the post-merge coverage goal described by the workflow header and
objective.
In @.shellcheckrc:
- Around line 36-37: The ShellCheck suppressions for SC3043 and SC3050 are
currently applied repo-wide in .shellcheckrc, but they only relate to bashisms
used by demo_build.sh. Move or scope these disables to demo_build.sh, or update
demo_build.sh to use bash so the rest of the repo keeps ShellCheck coverage; use
the demo_build.sh script and the .shellcheckrc entries as the places to adjust.
In `@demo_build.sh`:
- Around line 821-824: The `$FILESSERVEDIR` setup in the web file directory
creation step should be idempotent like the earlier directory creation logic.
Update the `mkdir` call used before the `cd $FILESSERVEDIR` flow to use the same
`-p` behavior as the other directory setup, so reruns do not fail if the
directory already exists or the parent path is missing.
In `@tools/build-tests/integration/run-scenario.sh`:
- Around line 39-40: The container images used in run-scenario.sh are currently
pinned only by mutable tags, which can make the golden tests flaky when upstream
republishes images. Update the IMAGE assignment and the other image references
in the same script to use digest-pinned image@sha256 identifiers instead of
tags, and keep any future tag changes explicit by changing the digest reference
in one place.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 81014f65-e2f8-4749-a6f6-cf8b4e919b7b
📒 Files selected for processing (33)
.github/workflows/build-tests-live.yml.github/workflows/build-tests.yml.shellcheckrcdemo_build.shtools/build-tests/fixtures/branch-pinned-master/expected/action-log.txttools/build-tests/fixtures/branch-pinned-master/expected/live-action-log.txttools/build-tests/fixtures/branch-pinned-master/inputs.envtools/build-tests/fixtures/branch-pinned-master/ip_map_branch.txttools/build-tests/fixtures/capsule-path-traversal/expected/fail.txttools/build-tests/fixtures/capsule-path-traversal/inputs.envtools/build-tests/fixtures/capsule-path-traversal/ip_map_branch.txttools/build-tests/fixtures/light-reset/expected/action-log.txttools/build-tests/fixtures/light-reset/inputs.envtools/build-tests/fixtures/light-reset/ip_map_branch.txttools/build-tests/fixtures/multi-demo/expected/action-log.txttools/build-tests/fixtures/multi-demo/inputs.envtools/build-tests/fixtures/multi-demo/ip_map_branch.txttools/build-tests/fixtures/release-packageserve/expected/action-log.txttools/build-tests/fixtures/release-packageserve/expected/live-action-log.txttools/build-tests/fixtures/release-packageserve/inputs.envtools/build-tests/fixtures/release-packageserve/ip_map_branch.txttools/build-tests/fixtures/tag-pinned-with-data/expected/action-log.txttools/build-tests/fixtures/tag-pinned-with-data/expected/live-action-log.txttools/build-tests/fixtures/tag-pinned-with-data/extra/git/demo_farm_openemr/pieces/demo_5_0_0_5.sqltools/build-tests/fixtures/tag-pinned-with-data/inputs.envtools/build-tests/fixtures/tag-pinned-with-data/ip_map_branch.txttools/build-tests/fixtures/up-for-grabs-fork/expected/action-log.txttools/build-tests/fixtures/up-for-grabs-fork/extra/git/demo_farm_openemr/pieces/demo_5_0_0_5.sqltools/build-tests/fixtures/up-for-grabs-fork/inputs.envtools/build-tests/fixtures/up-for-grabs-fork/ip_map_branch.txttools/build-tests/integration/run-all.shtools/build-tests/integration/run-scenario.shtools/build-tests/test.sh
Address two rabbit findings (PR openemr#148, applies to Phase 1): * Latent secret leak in live mode: _emit_action_log only redacted -p<pass> and rootpass=<pass>. The script also emits the github api token via `curl -H "Authorization: token $GITHUB_KEY_COMPOSER"` (rate-limit probe) and `composer config --auth github-oauth.github.com $KEY`. In dry-run those values are stubbed (placeholders have <> which %q-escapes to \<\>, dodging the new pattern's [A-Za-z0-9_]+ body match), but Phase 2 live runs with a real /home/openemr/ github-key mounted would otherwise commit the raw token to ACTION_LOG. Added two more sed patterns to cover both shapes. * test.sh docstring example was stale: the harness does `cp -a "$extra_dir/." "$work/"` which mirrors the work-tree layout. Fixtures need to put version.php at extra/web/openemr/version.php, not extra/openemr/version.php (which silently lands at the wrong root and never overrides $WEB/openemr/version.php). Phase 1 goldens are unchanged — the new redaction patterns don't fire against the dry-run stubs. Refs: openemr#146 (issue), openemr#147 (this PR), openemr#148 (Phase 2) Assisted-by: Claude Code
Rabbit-flagged: the fixture source step (`source inputs.env`) unconditionally clobbered the parsed CLI --image value with the fixture's INTEGRATION_IMAGE, so the advertised flag was effectively ignored whenever a fixture set the env var. Track whether --image came from the CLI; let the fixture value override only when the flag was not passed. Lets callers A/B-test a scenario against a different image (e.g., to bisect a flex tag regression) without editing the fixture. Refs: openemr#146 (issue), openemr#148 (this PR) Assisted-by: Claude Code
098ae80 to
1a21699
Compare
|
Thanks for the review. Quick notes on the three nitpicks I'm not addressing in this PR — each has a "fix not in this PR's scope" rationale: Pin images by digest ( Switch trigger from Scope Address the 3 actionable comments in:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/build-tests/integration/run-scenario.sh (1)
113-119: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the unused
PHP_VERSION_ABBRfixture assignment.demo_build.shgets this value from the flex image, so the runner-side copy ininputs.envis dead and makes the fixture contract look stricter than it is.🤖 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 `@tools/build-tests/integration/run-scenario.sh` around lines 113 - 119, Remove the unused PHP_VERSION_ABBR fixture assignment from the scenario runner setup. The copy in run-scenario.sh is dead because demo_build.sh derives this value from the flex image, so update the inputs.env sourcing block to stop declaring or relying on PHP_VERSION_ABBR and keep the fixture contract limited to the values actually consumed there.
🤖 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 `@tools/build-tests/integration/run-scenario.sh`:
- Around line 168-172: The MariaDB and OpenEMR container launches in the
scenario runner are using mutable tags without forcing refresh, so warm agents
can reuse stale cached images. Update the docker run calls in the scenario setup
to either add --pull=always or perform an explicit docker pull before starting
the containers, making sure both the MariaDB startup and the OpenEMR startup
paths use the refreshed image behavior.
---
Nitpick comments:
In `@tools/build-tests/integration/run-scenario.sh`:
- Around line 113-119: Remove the unused PHP_VERSION_ABBR fixture assignment
from the scenario runner setup. The copy in run-scenario.sh is dead because
demo_build.sh derives this value from the flex image, so update the inputs.env
sourcing block to stop declaring or relying on PHP_VERSION_ABBR and keep the
fixture contract limited to the values actually consumed there.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a8a2867-2f42-4820-be87-8c05ffbf13c5
📒 Files selected for processing (8)
.github/workflows/build-tests-live.ymldemo_build.shtools/build-tests/fixtures/branch-pinned-master/expected/live-action-log.txttools/build-tests/fixtures/release-packageserve/expected/live-action-log.txttools/build-tests/fixtures/tag-pinned-with-data/expected/live-action-log.txttools/build-tests/integration/run-all.shtools/build-tests/integration/run-scenario.shtools/build-tests/test.sh
✅ Files skipped from review due to trivial changes (3)
- tools/build-tests/fixtures/release-packageserve/expected/live-action-log.txt
- tools/build-tests/fixtures/branch-pinned-master/expected/live-action-log.txt
- tools/build-tests/fixtures/tag-pinned-with-data/expected/live-action-log.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/build-tests-live.yml
- tools/build-tests/integration/run-all.sh
- tools/build-tests/test.sh
- demo_build.sh
| docker run --detach --name "$MARIADB" \ | ||
| --network "$NETWORK" \ | ||
| --env "MYSQL_ROOT_PASSWORD=hey" \ | ||
| --ulimit nofile=65536:65536 \ | ||
| mariadb:10.6 \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the runner's Docker CLI supports the proposed pull policy.
docker run --help | grep -F -- '--pull' || {
echo "Docker CLI lacks docker run --pull support; use explicit docker pull commands instead." >&2
exit 1
}Repository: openemr/demo_farm_openemr
Length of output: 290
🌐 Web query:
Docker run --pull always documentation mutable tags cached local image behavior
💡 Result:
When you use the --pull always flag with the docker run command, Docker will unconditionally attempt to pull the specified image from the registry before creating and running the container, regardless of whether a version of that image already exists in your local cache [1][2]. This behavior is distinct from the default policy (missing), which only pulls the image if it is absent from the local cache [1]. Because --pull always forces a registry request, it effectively addresses the issue of mutable tags—where a tag like:latest or:stable may be updated in the registry—by ensuring that the local image cache is updated to match the current remote version before execution [1][2]. Key points regarding this behavior include: 1. Overwriting Local Images: If you have a locally built image with the same name and tag as a registry image, --pull always will overwrite your local image with the version fetched from the registry [1]. This can be problematic if you are attempting to test a locally built image that has not yet been pushed [1]. 2. Registry Efficiency: Modern Docker versions are optimized to minimize unnecessary downloads [3]. When you initiate a pull, Docker checks the manifest (often via a HEAD request) to determine if the local image digest matches the one in the registry [3]. If they match, the actual image layers are not re-downloaded [3]. 3. Alternative for Consistency: If you require absolute consistency and wish to avoid the ambiguity of mutable tags entirely, it is recommended to pull and run images by their immutable digest (e.g., image@sha256:...) rather than by tag [4][5][6]. Reference by digest guarantees that the container uses a specific, immutable version of the image regardless of what the tag points to in the registry [4][5].
Citations:
- 1: https://docs.docker.com/reference/cli/docker/container/run/
- 2: https://stackoverflow.com/questions/55941881/docker-run-vs-pull
- 3: Add option to docker pull to use local cache first without contacting Docker Hub docker/cli#6082
- 4: https://docs.docker.com/reference/cli/docker/image/pull/?highlight=docker&highlight=pull
- 5: docker run doesn't pull down latest image if the image exists locally moby/moby#13331
- 6: docker run doesn't pull down latest image if the image exists locally moby/moby#13331
Pull the mutable images before both runs. Add --pull=always (or an explicit docker pull) to the MariaDB and OpenEMR docker run calls so warm runners don’t reuse stale cached tags.
🤖 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 `@tools/build-tests/integration/run-scenario.sh` around lines 168 - 172, The
MariaDB and OpenEMR container launches in the scenario runner are using mutable
tags without forcing refresh, so warm agents can reuse stale cached images.
Update the docker run calls in the scenario setup to either add --pull=always or
perform an explicit docker pull before starting the containers, making sure both
the MariaDB startup and the OpenEMR startup paths use the refreshed image
behavior.
Rabbit-flagged. The fixture inputs.env carries PHP_VERSION_ABBR
because Phase 1 dry-run needs it (the script writes to
/etc/php\${ABBR}/conf.d/ which has no flex image to provide a value).
Phase 2 live runs inside the openemr flex container which bakes in
the right value, and run-scenario.sh never passes it via --env --
so the default-and-source dance here was a contract the runner
didn't actually have. Comment block explains why the var is kept in
fixtures despite not being read here.
Refs: openemr#146, openemr#148
Assisted-by: Claude Code
Wall time drops from ~25 min (sequential) to ~8-10 min (slowest of
3 cells). Each scenario runs on its own ubuntu-24.04 runner, so:
- Composer/npm/webpack/InstallerAuto across the three scenarios
run truly in parallel rather than serialized.
- Each cell gets a fresh VM, so no leftover state (docker layer
cache, mariadb data dir, /tmp clutter) from scenario N can
affect scenario N+1.
- One failing cell does NOT cancel its siblings (fail-fast: false).
Debugging is easier when you can see every result.
Cost: ~5 extra GitHub Actions minutes per run (3 cells x 10 min vs
1 runner x 25 min). Worth it during PR development where wall time
matters; still fine post-merge when this becomes nightly.
run-all.sh stays as the local-execution wrapper -- now annotated
with a "MUST stay in sync with workflow matrix" comment so anyone
adding/removing a scenario updates both places.
Refs: openemr#146 (issue), openemr#148 (this PR)
Assisted-by: Claude Code
The matrix conversion dropped wall time to ~10 min (slowest cell), which is now tractable as blocking per-PR CI. Per-PR catches the "PR-introduced regression" class; schedule catches the "upstream image bump on master" class that no PR triggers. Both paths are cheap on a low-volume repo, and they cover different failure modes. 08:00 UTC is 1h after derive-ip-map's daily reconcile (07:00) so the two scheduled flows don't race on the same window. Drops the pre-merge "TODO swap pull_request for schedule" note that was premised on a 25-min sequential wall time -- that constraint is gone, so per-PR can stay. Refs: openemr#146 (issue), openemr#148 (this PR) Assisted-by: Claude Code
…d.sh (#146) (#147) * feat(build-tests): add Phase 1 dry-run regression suite for demo_build.sh Phase 1 of issue #146: per-PR regression coverage that catches the PR #142 (printf '-' dash) and PR #145 (SC2115 rm -fr footguns) class of bug at PR time in seconds, rather than waiting for the nightly host reset to surface them. Three pieces: - demo_build.sh: additive --dry-run flag + ACTION_LOG emitter (run_action / run_action_sh / run_action_capture helpers). With no flag and no ACTION_LOG env, helpers degrade to plain exec -- production behavior is unchanged. - tools/build-tests/: fixture-test harness mirroring the tools/auto-derive/fixtures-and-tests/ pattern. 6 scenarios cover branch-pinned/tag-pinned/release-packageserve/fork-URL/ light-reset/multi-demo paths. - .github/workflows/build-tests.yml: runs the suite on every PR that touches demo_build.sh or tools/build-tests/**. Phase 2 (separate PR) will add a slower nightly job that actually executes demo_build.sh end-to-end against a compose stack, reusing the same fixtures + action-log emitter, and asserting the dry-run log matches what live execution produces -- the integrity check that keeps Phase 1 honest. Refs: #146 Assisted-by: Claude Code * feat(build-tests): address rabbit review + add capsule-path-traversal fixture Rabbit-flagged fixes: - .github/workflows/build-tests.yml: drop the stale "includes derive-ip-map.yml" comment that didn't match the actual paths filter (and didn't need to -- changes to that workflow file don't affect build behavior). - demo_build.sh: add a _emit_action_log helper that redacts `-p<password>` (mariadb root pass flag) and `rootpass=<password>` from action-log lines before append. Best-effort; documented that ACTION_LOG should not be enabled in production without further review. Phase 2 live-run work will extend this with composer auth tokens and Authorization headers when those become real values rather than dry-run stubs. - demo_build.sh: document the trust assumption on run_action_sh (wrapped strings come from ip_map_branch.txt + docker env, not user input). Eval is intentional for pipes/redirects; refactor is out of scope for this PR. - demo_build.sh: unwrap two `mkdir` calls in the packageServe block so subsequent `cd` commands work in dry-run, where the intervening rsync is skipped. Same pattern already used for $WEB/log and $OPENEMR earlier in the script. New fixture: - capsule-path-traversal: synthetic ip_map row with `capsule` column = "../etc", exercising the case statement guard PR #145 added (demo_build.sh lines ~617-622). Pins the rejection so a future regression that drops the guard fails CI -- otherwise it would silently re-enable path traversal in dry code, since no production row currently sets `capsule`. - tools/build-tests/test.sh: extends harness with the `expected/fail.txt` pattern (mirrors auto-derive harness): substring check on stderr/stdout + assert non-zero exit. All 7 fixtures pass. Verified negative test: stripping the case guard from demo_build.sh fails ONLY capsule-path-traversal. Refs: #146 (issue), #147 (this PR) Assisted-by: Claude Code * fix(build-tests): rabbit-review-2 fixes for test.sh harness Two findings from rabbit's second review pass: - Fail-expected check used `cat $stdout $stderr | grep -qF`, which under `set -o pipefail` (enabled at the top of the script) can flip a real PASS into a false FAIL: grep closes stdin on the first match, cat dies on SIGPIPE, pipeline exits non-zero. Read the files directly with `grep -qF -- $msg $stdout $stderr` — no pipe, no false negative. - normalize_action_log was called unconditionally even when the script produced no action log. If a future regression makes demo_build.sh exit 0 without writing ACTION_LOG, `sed -i` on the missing file would trip `set -e` and abort the whole harness mid-run, killing later fixtures and the summary. Added an explicit `[[ -f $action_log ]]` guard that fails just the affected fixture with a clear message and lets the run finish. Refs: #146 (issue), #147 (this PR) Assisted-by: Claude Code * fix(build-tests): redaction false-positive on --unsafe-perm The `-p<pass>` redaction regex matched anywhere in a line, including inside `--unsafe-perm` (npm flag), turning legitimate command args into `--unsafe-p<REDACTED>` in the action log. Surfaced by an in-progress Phase 2 smoke run — first time live mode exercised the npm install branch end-to-end. Anchor the pattern to start-of-line or whitespace so it only matches the standalone mariadb `-p<password>` flag. Regen the 6 goldens that contained the false-positive form. Refs: #146 (issue), #147 (this PR) Assisted-by: Claude Code * fix(build-tests): extend redaction + fix fixture-extras path docstring Address two rabbit findings (PR #148, applies to Phase 1): * Latent secret leak in live mode: _emit_action_log only redacted -p<pass> and rootpass=<pass>. The script also emits the github api token via `curl -H "Authorization: token $GITHUB_KEY_COMPOSER"` (rate-limit probe) and `composer config --auth github-oauth.github.com $KEY`. In dry-run those values are stubbed (placeholders have <> which %q-escapes to \<\>, dodging the new pattern's [A-Za-z0-9_]+ body match), but Phase 2 live runs with a real /home/openemr/ github-key mounted would otherwise commit the raw token to ACTION_LOG. Added two more sed patterns to cover both shapes. * test.sh docstring example was stale: the harness does `cp -a "$extra_dir/." "$work/"` which mirrors the work-tree layout. Fixtures need to put version.php at extra/web/openemr/version.php, not extra/openemr/version.php (which silently lands at the wrong root and never overrides $WEB/openemr/version.php). Phase 1 goldens are unchanged — the new redaction patterns don't fire against the dry-run stubs. Refs: #146 (issue), #147 (this PR), #148 (Phase 2) Assisted-by: Claude Code
Stacked on openemr#147 (Phase 1 dry-run suite); GitHub auto-retargets to master when Phase 1 lands. tools/build-tests/integration/run-scenario.sh stands up mariadb:10.6 + openemr/openemr:flex-3.22-php-8.2 on an isolated docker network, runs demo_build.sh for real (composer install, npm install, webpack themes, openemr install, mariadb populate, apache start), then asserts: 1. "Demo install script is complete" marker fires within the 15-min timeout (i.e., the full happy path including the slow phases dry-run skips). 2. Every expected database exists in mariadb with a `globals` table (sanity check that InstallerAuto actually ran). 3. Captured action log matches the fixture's golden (expected/live-action-log.txt), after normalizing per-run- varying values: mariadb container hostname -> <MARIADB_HOST>, randomly-picked CSS theme -> <LIVE_RANDOM_THEME>. The -p<pass> / rootpass=<pass> redaction inherits from Phase 1's helper. run-all.sh wraps the 3 live scenarios. The 4 remaining Phase 1 fixtures stay dry-run-only (cost/signal rationale documented inline): - light-reset: same composer/npm work as branch-pinned-master with no incremental signal (the lightReset gate is purely control-flow, already covered by action-log diff) - multi-demo: 3x the composer/npm cost to prove loop iteration that action-log diff already proves - capsule-path-traversal: fail-expected, exits before any side effect - up-for-grabs-fork: depends on a community fork's branch state (flaky-by-design, not our regression to catch) Real demo data SQL from $REPO_ROOT/pieces/ is overlaid onto the fixture's pieces/ at run time (Phase 1 fixtures carry empty markers for the SQL since dry-run wraps the mariadb load; live needs actual content -- without it the demo-data DROP+RELOAD cycle wipes the schema InstallerAuto installed). CI workflow: build-tests-live.yml. - During this PR's development: on: pull_request + workflow_dispatch so we get per-push feedback while iterating. - Before merge: switch to on: schedule (cron '0 8 * * *', 1h after the auto-derive reconcile job) and drop pull_request. The workflow's comment block flags this explicitly so it can't be forgotten. What's deliberately deferred to follow-ups: - actions/cache for ~/.composer/cache + ~/.npm (would shave ~10 min off nightly). - Direct diff against Phase 1 dry-run goldens (needs Phase 1 goldens redesigned with abstract path tokens; non-trivial scope creep). - GitHub auth for composer (unauthenticated 60/hour limit is fine for one nightly run). Refs: openemr#146 (issue), openemr#147 (Phase 1) Assisted-by: Claude Code
Rabbit-flagged: the fixture source step (`source inputs.env`) unconditionally clobbered the parsed CLI --image value with the fixture's INTEGRATION_IMAGE, so the advertised flag was effectively ignored whenever a fixture set the env var. Track whether --image came from the CLI; let the fixture value override only when the flag was not passed. Lets callers A/B-test a scenario against a different image (e.g., to bisect a flex tag regression) without editing the fixture. Refs: openemr#146 (issue), openemr#148 (this PR) Assisted-by: Claude Code
Rabbit-flagged. The fixture inputs.env carries PHP_VERSION_ABBR
because Phase 1 dry-run needs it (the script writes to
/etc/php\${ABBR}/conf.d/ which has no flex image to provide a value).
Phase 2 live runs inside the openemr flex container which bakes in
the right value, and run-scenario.sh never passes it via --env --
so the default-and-source dance here was a contract the runner
didn't actually have. Comment block explains why the var is kept in
fixtures despite not being read here.
Refs: openemr#146, openemr#148
Assisted-by: Claude Code
Wall time drops from ~25 min (sequential) to ~8-10 min (slowest of
3 cells). Each scenario runs on its own ubuntu-24.04 runner, so:
- Composer/npm/webpack/InstallerAuto across the three scenarios
run truly in parallel rather than serialized.
- Each cell gets a fresh VM, so no leftover state (docker layer
cache, mariadb data dir, /tmp clutter) from scenario N can
affect scenario N+1.
- One failing cell does NOT cancel its siblings (fail-fast: false).
Debugging is easier when you can see every result.
Cost: ~5 extra GitHub Actions minutes per run (3 cells x 10 min vs
1 runner x 25 min). Worth it during PR development where wall time
matters; still fine post-merge when this becomes nightly.
run-all.sh stays as the local-execution wrapper -- now annotated
with a "MUST stay in sync with workflow matrix" comment so anyone
adding/removing a scenario updates both places.
Refs: openemr#146 (issue), openemr#148 (this PR)
Assisted-by: Claude Code
The matrix conversion dropped wall time to ~10 min (slowest cell), which is now tractable as blocking per-PR CI. Per-PR catches the "PR-introduced regression" class; schedule catches the "upstream image bump on master" class that no PR triggers. Both paths are cheap on a low-volume repo, and they cover different failure modes. 08:00 UTC is 1h after derive-ip-map's daily reconcile (07:00) so the two scheduled flows don't race on the same window. Drops the pre-merge "TODO swap pull_request for schedule" note that was premised on a 25-min sequential wall time -- that constraint is gone, so per-PR can stay. Refs: openemr#146 (issue), openemr#148 (this PR) Assisted-by: Claude Code
9de9553 to
40242c6
Compare
…s) (#149) PR A of the shellcheck ratchet cleanup, following the analysis in issue #146. The build-tests suites landed in #147/#148 are the regression net that makes this safe to do in one PR. Disables dropped (9): - SC3014, SC3028, SC3030, SC3037, SC3043, SC3050, SC3054 (7 POSIX-sh warnings) -- all caused by demo_build.sh carrying a stale `#!/bin/sh` shebang while using bash features (arrays, `[[`, `==`, `RANDOM`, `local`, `printf %q`) throughout. Production already invokes via `bash -c demo_build.sh` from docker/scripts/demoLibrary.source, so the runtime call shape doesn't change. - SC1001 (1 violation, demo_build.sh:205): `\=` is a regular `=` in this context (the backslash in `cut -d \= -f 2` was a no-op). - SC2046 (1 violation, demo_build.sh:205): unquoted `$()` in `collect_var` (also covered SC2005 -- useless `echo $(cmd)`). Refactored to drop the unnecessary `echo` wrapper; the inner pipeline already produces the value the caller captures via `$(collect_var ...)`. - SC2048 (1 violation, demo_build.sh:314): `${demosGo[*]}` → `"${demosGo[@]}"` for the for-loop. Same behavior for entries without spaces (all current entries are "empty"/"a"/.../"i"); canonical bash array iteration. - SC2005 (1 violation, demo_build.sh:205): covered above. - SC2016 (2 violations): literal `$v_major` (test.sh:120) and literal `$1` (derive.sh:1330) inside single-quoted printf formats -- intentional, the `$` characters are emitted into output. Inline `# shellcheck disable=SC2016` per site. - SC2001 (3 violations, in 2 files): `echo "$var" | sed 's/^/...'` indent-prefix idiom. Inline `disable=SC2001` per site rather than rewriting to awk -- there are 14 other `sed 's/^/...'` sites in the same files reading from cmd/file output (not flagged), and rewriting only the 3 flagged sites would split the codebase between two idioms. Matching comments at each site explain the choice. - SC2129 (0 violations): rc comment overcounted; no actual flags. - SC2196 (0 violations): same -- the 2 `egrep` calls flagged earlier were inside demo_build.sh strings passed to mariadb-dump | egrep, not on bare lines shellcheck inspects. Disables retained for follow-up PRs (per the issue #146 ratchet plan): SC1090, SC2006, SC2034, SC2086, SC2155, SC2164. Verification: - `shellcheck --check-sourced --external-sources` against every *.sh and *.source in the repo (excluding fixtures) -- clean. - `./tools/build-tests/test.sh` -- 7/7 PASS (Phase 1 dry-run regression suite catches behavioral surprises from the cleanup). - `./tools/auto-derive/fixtures-and-tests/test.sh` -- 8/8 PASS. Refs: #146 (parent issue closed but discussion is the plan source), #139 (workflow), #145 (prior SC2115 ratchet step). Assisted-by: Claude Code
Rabbit-flagged regression in this PR. The previous code at startFarm.sh:68 and restartFarm.sh:32 had a comment marking `wkhtmltopdf-openemr` as optional. The unguarded `cd ~/...` silently failed when the directory was absent, so the subsequent `git pull` failed too -- but the script kept going. That silent failure WAS the "skip if absent" semantic. Adding `|| exit 1` (the SC2164 fix) turned the documented-optional repo into a hard requirement: a fresh host without the wkhtmltopdf checkout would exit before starting any docker containers. The build-tests suite (openemr#147/openemr#148) only covers demo_build.sh, so this regression wouldn't surface in CI -- the docker/scripts/* orchestration scripts run on the production EC2 host and were explicitly out-of-scope when openemr#146 was planned. Rabbit's review was the only layer that caught this. Fix: wrap both sites with `if [ -d ~/wkhtmltopdf-openemr ]` guards. Keeps the cd-failure-is-fatal semantic (so a stale dir without git history still exits loudly), but skips the block entirely when the optional repo isn't there. Refs: openemr#146 (issue), openemr#150 (this PR) Assisted-by: Claude Code
* ci(shellcheck): drop SC2164 disable + guard all 25 cd sites PR B of the ratchet cleanup. SC2164 flags `cd $X` without `|| exit` (or `|| return` inside a function) -- silent cd failures leave the script running in the wrong directory and any subsequent `rm -fr`, `git pull`, `tar -czf`, or similar operates somewhere it shouldn't. Same shape that caused the SC2115 footguns (#145) but harder to spot because the wrong directory is downstream of the failure. All 25 sites guarded: - demo_build.sh (9 sites): top-level cd inside the demosGo loop body. `|| exit 1` -- the surrounding `for demo in ...` is the only retry point, and entering the next iteration with the wrong $PWD would scramble per-iteration paths. - docker/scripts/startFarm.sh (6 sites): repo update steps (`cd ~/<repo>; git pull; cd ~/`). `|| exit 1` -- if the repo dir is missing, the subsequent `git pull` would silently pull into $HOME or wherever cd left us. - docker/scripts/restartFarm.sh (6 sites): same pattern. - docker/scripts/refreshWebsite.sh (2 sites): same pattern. - docker/scripts/demoLibrary.source (2 sites): inside `snapshotDemo()` function, so `|| return 1` to bubble the failure up to the caller. Verification: - `shellcheck --check-sourced --external-sources` against every *.sh and *.source in the repo (excluding fixtures) -- clean. - `./tools/build-tests/test.sh` -- 7/7 PASS (Phase 1 dry-run suite catches behavioral surprises -- the cd guards run before every wrapped command, so a regression where cd silently failed would now fail loudly). - `./tools/auto-derive/fixtures-and-tests/test.sh` -- 8/8 PASS. Disables remaining: SC1090, SC2006, SC2034, SC2086, SC2155 (5 of the original 15 from before this cleanup series). Refs: #146 (parent issue), #149 (prior shellcheck cleanup PR A) Assisted-by: Claude Code * fix(scripts): preserve optional-repo semantic on wkhtmltopdf-openemr Rabbit-flagged regression in this PR. The previous code at startFarm.sh:68 and restartFarm.sh:32 had a comment marking `wkhtmltopdf-openemr` as optional. The unguarded `cd ~/...` silently failed when the directory was absent, so the subsequent `git pull` failed too -- but the script kept going. That silent failure WAS the "skip if absent" semantic. Adding `|| exit 1` (the SC2164 fix) turned the documented-optional repo into a hard requirement: a fresh host without the wkhtmltopdf checkout would exit before starting any docker containers. The build-tests suite (#147/#148) only covers demo_build.sh, so this regression wouldn't surface in CI -- the docker/scripts/* orchestration scripts run on the production EC2 host and were explicitly out-of-scope when #146 was planned. Rabbit's review was the only layer that caught this. Fix: wrap both sites with `if [ -d ~/wkhtmltopdf-openemr ]` guards. Keeps the cd-failure-is-fatal semantic (so a stale dir without git history still exits loudly), but skips the block entirely when the optional repo isn't there. Refs: #146 (issue), #150 (this PR) Assisted-by: Claude Code
The README hadn't been touched to reflect several years of demo_farm work. Rewritten to match current reality: Workflow badges added at top: - ShellCheck (post-cleanup: rc has zero disable= lines) - build-tests (dry-run) -- per-PR fixture suite from #147 - build-tests (live integration) -- nightly + per-PR matrix from #148 - Derive ip_map (reconcile) -- auto-derive bot from #135 ip_map_branch.txt column docs: - Converted from bullet list to table for readability. - Marked `branch_tag` (col 13) as deprecated -- matches the header annotation, the demo_build.sh comment ("col 13 (branch_tag) is deprecated; ... `git clone --branch` works for both"), and the fact that every production row now carries `0` there. - `fun_stuff`: documented actual code behavior. `1` = 36-theme picker, `2` = 6-theme picker, any other non-zero value falls through to the 36-theme picker via the `else` branch. Previously the README said "1 for <6.0, 2 for 6.0+" which reads like a version gate but is really a picker-selection. - `pass_reset`: documented that set_pass.php actually supports modes 1-4 (3/4 are 6.0.0+ variants), production uses 4, AND the mechanism is currently disabled pending openemr/openemr#5991. Column values are kept aligned with intended re-enable. - `capsule`: added the path-traversal guard mention (added in #145) as a footnote-style note. New sections: - "Regression tests" -- describes the dry-run + live-integration suites from #147/#148 and the auto-derive fixtures. - "Auto-derive bot" -- describes the daily reconcile flow from #135 including the release-targets-changed cross-repo dispatch. - "ShellCheck" -- notes the ratchet cleanup completion (PRs #149 through #157) and the current-clean state. Kept intact: - "UP FOR GRABS" onboarding steps (5-step process; still current). - "How to restart the demo farm after a host reboot" section (order + scripts still current). Trimmed the "watch this YouTube video 15 times" line since it dated the page. Intro paragraph rewritten to name the actual URL structure (demo.openemr.io etc.) and describe what a cluster/subdemo is, rather than the previous single-line description. License section preserved. Assisted-by: Claude Code
Summary
Phase 2 of #146 — live end-to-end integration tests for
demo_build.sh.Builds on #147 (Phase 1's dry-run suite, merged as 91477d1). The diff here is rebased onto master and shows only the Phase 2 changes.
tools/build-tests/integration/run-scenario.shstands upmariadb:10.6+ anopenemr/openemr:flex-3.22-php-8.2container on an isolated docker network, runsdemo_build.shfor real (composer install, npm install, webpack themes build, OpenEMR install, mariadb populate, apache start), then asserts:globalstable (sanity check thatInstallerAuto.phpactually ran).expected/live-action-log.txtgolden (after normalizing the per-run-varying mariadb container hostname and the randomly-picked theme value).Same fixtures Phase 1 uses; no fixture duplication. Container + network are cleaned up on EXIT (success or failure).
Scenarios run live
Three of Phase 1's seven, picked to exercise distinct expensive behavior:
branch-pinned-mastertag-pinned-with-data$v_major≥6)release-packageserveThe 4 remaining stay dry-run-only —
light-reset/multi-demoare too expensive per signal added (lightReset just gates the post-loop block; multi-demo costs 3× composer for what action-log diff already proves);capsule-path-traversalexits before any side effect;up-for-grabs-forkdepends on a community fork's branch state (flaky-by-design).CI: matrix-parallel + scheduled
.github/workflows/build-tests-live.ymlruns each scenario on its ownubuntu-24.04runner viastrategy.matrix. Wall time = slowest cell (~10 min) instead of sum (~25 min sequential).fail-fast: falseso a single cell failing doesn't cancel its siblings — debugging needs to see all three results.Actual matrix timings observed on this PR:
live (branch-pinned-master)live (tag-pinned-with-data)live (release-packageserve)Three triggers, complementary:
pull_request— per-push feedback on PRs that touchdemo_build.sh/tools/build-tests/**. Made viable by the matrix cutting wall time to ~10 min.schedule: '0 8 * * *'— daily 08:00 UTC, 1h after the auto-derive reconcile. Catches the class no PR triggers: upstream image bumps (Alpine, mariadb 10.6 → 10.7, openemr/flex) that breakdemo_build.shassumptions without anyone editing it.workflow_dispatch— manual reruns for debugging a flaky scheduled run.What this PR deliberately does NOT do
expected/live-action-log.txtinstead. A direct diff would need Phase 1 goldens redesigned with abstract path tokens (<OPENEMR>instead of<WORK>/web/openemr) — non-trivial scope creep. The two-golden split lets human reviewers diff both files manually if they want the "dry-run vs live drift" signal. (Real example surfaced during smoke testing: dry-run logsrm -f ofc_upload_image.phpbecause the fixture pre-creates the marker file; live doesn't, because upstream openemr already removed that file. Both behaviors are correct — the script handles both presence and absence — but the goldens differ on that line.)actions/cachefor~/.composer/cache+~/.npmis a small follow-up that would bring per-cell time down further.run-scenario.shalready prints container logs + setup-log tail on every failure path; visible in the workflow log.github-keyis mounted in CI, so the script'scat /home/openemr/github-keyfails (silently, noset -e), the rate-limit curl returns 60-remaining from an unauthenticated call, and the script takes the "not using composer github api token" branch. Works for one nightly run; risk if rate-limited.Rabbit-flagged items addressed mid-review
1a21699—--imageCLI flag now takes precedence over fixture'sINTEGRATION_IMAGE(lets callers A/B-test a scenario against a different image without editing the fixture).625cda9— drop deadPHP_VERSION_ABBRsourcing inrun-scenario.sh(the flex image provides its own; runner-side source was a contract this script doesn't actually have).fa9e75b— matrix conversion (this).9de9553— schedule trigger added; pre-mergepull_request-drop TODO retired.Test plan
./tools/build-tests/integration/run-scenario.sh branch-pinned-master— local smoke (PASS)./tools/build-tests/integration/run-all.shlocally — all 3 scenarios (PASS)expected/live-action-log.txt) — production paths, normalized hostnames + theme + redacted secrets, expected branching coveredRefs: #146 (issue), #147 (Phase 1)
🤖 Generated with Claude Code