feat(openemr-cmd): pass HOST_UID/HOST_GID through compose to openemr container#840
Merged
bradymiller merged 4 commits intoJun 26, 2026
Conversation
…container Completes the cross-repo HOST_UID story. openemr/openemr#12642 landed the flex entrypoint change that honors HOST_UID/HOST_GID (apache adopts the host uid before any chown work). This PR wires the openemr-devops side: wt_write_override emits HOST_UID/HOST_GID env vars in the openemr service block, sourced from `id -u` / `id -g` at worktree-add time. Result for non-uid=1000 hosts (CI runners at uid=1001, multi-user dev boxes, distros that start uids at 1001+, macOS via Docker Desktop): apache inside the container now writes bind-mount files with the host's uid. Host-side writes (`git commit`, IDE edits, `worktree regen`, `worktree set-env`) work without any chown dance. The uid-mismatch class of bugs (openemr#833, openemr#836, openemr#838) is eliminated end-to-end. Backwards compatible: pre-HOST_UID-aware images silently ignore the env vars (current behavior preserved). Emitted unconditionally even on uid=1000 hosts because the entrypoint adoption block is a sub-millisecond no-op there — the consistent behavior across hosts is worth more than the savings. CI workarounds removed (now obsolete): - "One-shot chown back to runner (enables host-side regen + set-env)" in worktree-lifecycle-e2e (added in openemr#834) - "One-shot chown back to runner (enables host-side writes + git commits)" in prek-workflow-e2e (added in openemr#836) Both replaced with NOTE comments explaining what used to live there and why the chown is no longer needed. The openemr#833 auto-chown block in cmd_worktree_remove is kept as a back-compat shim for users on pre-HOST_UID images. Inline comment updated to reflect its new status (no-op when image honors HOST_UID, useful for older images and for any drift cases the entrypoint adoption misses). Tests: tests/bats/openemr-cmd/host_uid_compose.bats pins the new behavior — 3 cases (env vars present, structurally nested under services.openemr.environment, emitted for all 3 envs). Assisted-by: Claude Code
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds ChangesOpenEMR UID and sourcing flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…urce-only guard; bump VERSION Two cleanups bundled: (1) The hermetic bats suite used to source openemr-cmd's function definitions via `eval "$(head -n OC_SCRIPT_FUNCS_END script)"` with a magic line-number constant that had to be bumped every time the script grew/shrank near end-of-functions. A drift sentinel test caught stale constants but the bump itself was easy to forget — which broke CI on openemr#838 and again now on this branch, distracting from the actual review surface. Replace with a __OPENEMR_CMD_SOURCE_FUNCS_ONLY=1 env-var guard: the script returns early from sourcing right after the last function def when the flag is set; direct invocation (./openemr-cmd) is unaffected since the flag isn't in the runtime environment. Tests that want only function defs set the flag and source — no magic number, no drift, no maintenance. - openemr-cmd: source guard added before USAGE_EXIT_CODE=13 - helpers.bash: oc_source_funcs uses the env var; OC_SCRIPT_FUNCS_END constant removed - helpers_pure.bats: oc_run_in_funcs drops the funcs_end arg; the drift sentinel test replaced with two guard-behavior tests (sourcing with the flag defines funcs but skips dispatch; direct execution is unaffected by the flag) - copy_base_env.bats: oc_run_in_funcs drops the funcs_end arg - state.bats, state_lock.bats, state_lock_crash_recovery.bats, worktree_add_base.bats: inline eval-of-head replaced with source-with-flag pattern (2) Bump VERSION 1.0.48 → 1.0.49 for the HOST_UID passthrough + this refactor. Was missed in the initial openemr#840 push. Assisted-by: Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/bats/openemr-cmd/helpers_pure.bats (1)
24-37: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winStale parameter arity —
tmp_rootis now always empty,OPENEMR_ROOTset blank.The callers were updated to the new 3-argument shape (
oc_run_in_funcs '<snippet>' "$SCRIPT" "$TMP_ROOT"), dropping the oldfuncs_endargument. But this helper still declareslocal funcs_end=$3/local tmp_root=$4, so$TMP_ROOTlands infuncs_endandtmp_rootresolves to empty — makingenv OPENEMR_ROOT="${tmp_root}"set an empty root. The siblingcopy_base_env.batshelper was already corrected tolocal tmp_root=$3; this one was missed.🐛 Proposed fix
oc_run_in_funcs() { local snippet=$1 local script_path=$2 - local funcs_end=$3 - local tmp_root=$4 + local tmp_root=$3 run env OPENEMR_ROOT="${tmp_root}" bash -c " set -euo pipefail __OPENEMR_CMD_SOURCE_FUNCS_ONLY=1 source '${script_path}' ${snippet} " }🤖 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 `@tests/bats/openemr-cmd/helpers_pure.bats` around lines 24 - 37, The oc_run_in_funcs helper has stale argument parsing after the call sites switched to three arguments, so OPENEMR_ROOT is being set from an empty tmp_root value. Update the helper signature/locals in oc_run_in_funcs to match the new 3-argument shape used by the callers, following the same pattern as the sibling helper in copy_base_env.bats, so script_path and tmp_root are read from the correct positions and env OPENEMR_ROOT receives the actual temp root.
🧹 Nitpick comments (1)
tests/bats/openemr-cmd/helpers_pure.bats (1)
30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment. The
# eval, not 'source <(...)'rationale no longer applies now that the helper sources the script directly rather thaneval-ing it. Consider updating to reference the__OPENEMR_CMD_SOURCE_FUNCS_ONLYmechanism to avoid confusing future readers.🤖 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 `@tests/bats/openemr-cmd/helpers_pure.bats` around lines 30 - 32, The inline comment in the helper setup is stale and still references the old eval/process-substitution behavior. Update the comment near the shell setup in helpers_pure.bats to describe the current direct-sourcing approach and mention the __OPENEMR_CMD_SOURCE_FUNCS_ONLY mechanism so readers understand why only functions are loaded.
🤖 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.
Outside diff comments:
In `@tests/bats/openemr-cmd/helpers_pure.bats`:
- Around line 24-37: The oc_run_in_funcs helper has stale argument parsing after
the call sites switched to three arguments, so OPENEMR_ROOT is being set from an
empty tmp_root value. Update the helper signature/locals in oc_run_in_funcs to
match the new 3-argument shape used by the callers, following the same pattern
as the sibling helper in copy_base_env.bats, so script_path and tmp_root are
read from the correct positions and env OPENEMR_ROOT receives the actual temp
root.
---
Nitpick comments:
In `@tests/bats/openemr-cmd/helpers_pure.bats`:
- Around line 30-32: The inline comment in the helper setup is stale and still
references the old eval/process-substitution behavior. Update the comment near
the shell setup in helpers_pure.bats to describe the current direct-sourcing
approach and mention the __OPENEMR_CMD_SOURCE_FUNCS_ONLY mechanism so readers
understand why only functions are loaded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b895dd52-8eeb-4323-a391-1455e2120683
📒 Files selected for processing (8)
tests/bats/openemr-cmd/copy_base_env.batstests/bats/openemr-cmd/helpers.bashtests/bats/openemr-cmd/helpers_pure.batstests/bats/openemr-cmd/state.batstests/bats/openemr-cmd/state_lock.batstests/bats/openemr-cmd/state_lock_crash_recovery.batstests/bats/openemr-cmd/worktree_add_base.batsutilities/openemr-cmd/openemr-cmd
…d values The override.yml golden fixtures didn't include the new HOST_UID / HOST_GID env vars that wt_write_override now emits, breaking the 3 golden assertion tests. Add the environment block to each env's golden (easy / easy-light / easy-redis) right before the volumes block, matching the live script output. Mask the uid/gid values to __HOST_UID__ / __HOST_GID__ in mask_paths so goldens stay stable across runner uids (CI = 1001, contributor dev boxes typically 1000, etc.) without UPDATE_GOLDENS needing to run on each contributor's machine. Assisted-by: Claude Code
…yyaml dep) Two macOS-specific failures from the previous push: (1) "source-funcs-only guard: direct execution is unaffected by the flag" expected exit 14 (VERSION_EXIT_CODE) but got 16 (DOCKER_CODE) on macOS. The script's docker-availability check runs before --version parsing — macOS GH-hosted runners don't have docker installed, so the script exits 16 before reaching --version. Fix: stub docker via oc_make_docker_stub_dir (already used by other hermetic tests), then run --version against the stubbed PATH. (2) "host_uid: env vars are nested under services.openemr.environment" used pyyaml which isn't on macOS runners (ModuleNotFoundError). Replace with awk that walks the override file linearly tracking the in_services/in_openemr/in_env state. Same structural assertion, no Python deps. Verified locally that the awk parses the real wt_write_override output correctly. Assisted-by: Claude Code
This was referenced Jun 26, 2026
bradymiller
added a commit
to bradymiller/openemr
that referenced
this pull request
Jun 26, 2026
Brief mention of the openemr-cmd HOST_UID/HOST_GID auto-export in two places where contributors and agents will look: - CONTRIBUTING.md: added a sub-bullet under the openemr-cmd intro explaining that bind-mounted files apache writes are host-owned for any host uid (transparent for new checkouts; one-shot sudo chown documented for legacy checkouts with root-owned cruft). - CLAUDE.md: added a Common Gotchas entry so future agents debugging EACCES errors on bind-mount edits go straight to "did you bypass openemr-cmd?" instead of tracing it themselves. Also notes the insane env hasn't been wired up yet (older image tags don't honor HOST_UID). The auto-export landed in openemr/openemr-devops#840 (worktree side) and openemr/openemr-devops#842 (non-worktree side); the entrypoint adoption it depends on landed in openemr#12642 (image side) and openemr#12647 (compose env declaration). Assisted-by: Claude Code
bradymiller
added a commit
to openemr/openemr
that referenced
this pull request
Jun 26, 2026
…12650) ## Summary Two small docs additions for the HOST_UID/HOST_GID auto-export that openemr-cmd now handles transparently (landed via openemr/openemr-devops#840 + #842; depends on #12642 entrypoint + #12647 compose env declarations, both also already landed). **CONTRIBUTING.md** — added a sub-bullet under the openemr-cmd intro at line 118. New contributors using openemr-cmd see the behavior is transparent for new checkouts; long-lived checkouts with root-owned cruft from older docker runs have a one-shot `sudo chown -R "$(id -u):$(id -g)" .` cleanup. **CLAUDE.md** — added a Common Gotchas entry so future agents debugging EACCES on bind-mount edits go straight to "did you bypass openemr-cmd?" instead of tracing it themselves. ## Scope 10 lines added across the two files. No code/behavior changes. ## What this does NOT cover - The insane env compose + image tags don't honor HOST_UID yet. Out of scope for this docs PR; tracked as a separate follow-up (extend HOST_UID to insane requires rebuilding 11+ openemr image variants + bumping SHA pins). The CLAUDE.md gotcha notes this explicitly so agents debugging insane env know not to be surprised. ## Test plan - [x] prek hooks (codespell, trailing-whitespace, end-of-file-fixer, conventional-commits) pass on the diff - [ ] Standard openemr CI passes (docs-only change, no functional impact expected) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
1 participant
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
Completes the cross-repo HOST_UID story started in openemr/openemr#12642 (entrypoint side, already merged + new flex image SHA pinned via dependabot #12644). This PR wires the openemr-devops side:
wt_write_overrideemitsHOST_UID/HOST_GIDenv vars in the openemr service block, sourced fromid -u/id -gat worktree-add / regen time.Result
For non-uid=1000 hosts (CI runners at uid=1001, multi-user dev boxes, distros that start uids at 1001+, macOS via Docker Desktop): apache inside the container now writes bind-mount files with the host's uid. Host-side writes (
git commit, IDE edits,worktree regen,worktree set-env) work without any chown dance. The uid-mismatch class of bugs that dominated #833 / #836 / #838 review cycles is eliminated end-to-end.For uid=1000 hosts (most Linux desktop developers): zero functional change. The env vars are emitted unconditionally because the entrypoint adoption block is a sub-millisecond no-op on uid=1000 and the consistent behavior across hosts is worth more than the savings.
Backwards compatible: pre-HOST_UID-aware images silently ignore the env vars.
CI workarounds removed (now obsolete)
Both
sudo chown -R "$(id -u):$(id -g)"steps intest-bats-openemr-cmd-real-docker.yml:worktree-lifecycle-e2e"One-shot chown back to runner (enables host-side regen + set-env)" — added in ci(openemr-cmd): e2e job titles match contents; add list/regen/set-env/auto-chown coverage #834prek-workflow-e2e"One-shot chown back to runner (enables host-side writes + git commits)" — added in test(openemr-cmd): e2e — prek install + real git commit; expand multi-concurrent to 3 envs #836Both replaced with
NOTE:comments documenting what used to live there and why the chown is no longer needed — for archaeology by future contributors.Kept as back-compat shim
The
#833auto-chown block incmd_worktree_removestays in place. Inline comment updated to reflect its new status (no-op when the image honors HOST_UID; still useful for users on pre-HOST_UID images and for any drift the entrypoint adoption might miss).Tests
New bats file
tests/bats/openemr-cmd/host_uid_compose.batspins three behaviors:wt_write_overrideemitsHOST_UIDandHOST_GIDenv vars in the override fileid -u/id -gat write timeservices.openemr.environment(structural check via yaml.safe_load)Test plan
git commitfrom the worktree works as runner)🤖 Generated with Claude Code
Summary by CodeRabbit
HOST_UID/HOST_GID) in the container environment.HOST_UID/HOST_GIDemission and stability of golden comparisons by masking host-specific values.