fix(docker): set sampleworks workspace home#253
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds HOME and XDG environment variables and sets WORKDIR to /home/dev in runtime images (including Dockerfile.astera), creates XDG/workspace directories, appends the interactive auto-cd snippet to both root and dev bashrcs, and updates tests to use a tmp_path-based runner script for deterministic workspace detection. ChangesContainer Workspace and Test Environment Setup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Updates the public Sampleworks runtime image (and the Astera overlay) to treat /home/dev as the runtime “home/workspace root”, aligning container defaults (HOME/XDG/workdir) with ACTL workspace pod expectations.
Changes:
- Set
HOME, XDG base dirs, andSHELLin the public runtime image and create the corresponding state directories under/home/dev. - Change the public runtime image
WORKDIRto/home/dev. - Explicitly set
WORKDIR /home/devin the Astera overlay to stay aligned with ACTL workspace images.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Dockerfile | Sets HOME/XDG/SHELL, creates /home/dev state dirs, and changes runtime WORKDIR to /home/dev. |
| Dockerfile.astera | Sets overlay WORKDIR to /home/dev to match the workspace home convention. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| XDG_DATA_HOME=/home/dev/.local/share \ | ||
| SHELL=/bin/bash | ||
|
|
||
| RUN mkdir -p /home/dev/.config /home/dev/.cache /home/dev/.local/share /home/dev/workspace |
There was a problem hiding this comment.
Addressed in 77ea7c7 by installing the ACTL auto-cd snippet into both /root/.bashrc and /home/dev/.bashrc. Keeping /root/.bashrc preserves the prior root-user behavior, and /home/dev/.bashrc matches the new HOME=/home/dev runtime environment.
marcuscollins
left a comment
marcuscollins
left a comment
There was a problem hiding this comment.
I think this is fine but please take a look at the Copilot comment about whether the .bashrc file also needs to be moved to /home/dev/. I honestly don't know the sequence of steps that leads to /root/.bashrc being executed, and whether/how changing HOME would alter that.
![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
🧹 Nitpick comments (1)
tests/runs/conftest.py (1)
13-32: ⚡ Quick winDocument the fixture's side effects in a NumPy-style docstring.
This fixture now mutates both process environment variables and
runner.WORKSPACE_GRID_SEARCH_SCRIPT, but the docstring does not spell that out and is not in the repo's required NumPy style.Suggested docstring shape
def force_pixi_argv(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: - """Keep argv assertions deterministic across dev and ACTL image environments.""" + """Keep argv assertions deterministic across dev and ACTL image environments. + + Parameters + ---------- + monkeypatch : pytest.MonkeyPatch + Isolates environment and module state for each test. + tmp_path : Path + Temporary directory used to point workspace script discovery at a + guaranteed-missing path. + + Notes + ----- + Deletes several ``SAMPLEWORKS_*`` environment variables, sets + ``SAMPLEWORKS_FORCE_PIXI=1``, and monkeypatches + ``sampleworks.runs.runner.WORKSPACE_GRID_SEARCH_SCRIPT``. + """As per coding guidelines, "Always include NumPy-style docstrings for every function and class." and "Always annotate complex array shapes and note side effects."
🤖 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/runs/conftest.py` around lines 13 - 32, Update the docstring for the fixture force_pixi_argv to a NumPy-style docstring that clearly documents its behavior and side effects: describe that it mutates process environment variables (clears SAMPLEWORKS_GRID_SEARCH_SCRIPT, SAMPLEWORKS_PIXI_PROJECT_DIR, unsets vars starting with "SAMPLEWORKS_" and ending with "_PYTHON", and removes/sets RUNTIME_PIXI, SAMPLEWORKS_ALLOW_RUNTIME_PIXI, SAMPLEWORKS_REQUIRE_PREBUILT_PIXI, SAMPLEWORKS_SKIP_ENV_PREPARE, and sets SAMPLEWORKS_FORCE_PIXI="1") and that it overwrites runner.WORKSPACE_GRID_SEARCH_SCRIPT to point at a tmp_path missing-workspace run_grid_search.py; include parameter descriptions for monkeypatch and tmp_path and a brief “Side effects” section listing the mutated environment variables and the modified runner.WORKSPACE_GRID_SEARCH_SCRIPT.
🤖 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 `@Dockerfile`:
- Around line 138-146: The interactive-shell auto-cd hook was written to root's
bashrc before HOME was changed to /home/dev, so after setting HOME the hook is
no longer sourced; modify the earlier step that installs the hook to
write/append it to $HOME/.bashrc (or use the HOME env when creating the file)
and ensure ownership/permissions match the dev user (chown $HOME and set
readable). In practice, update the command that previously targeted root's
.bashrc to target $HOME/.bashrc (or use the HOME variable) so the hook is
present for the new HOME and persists in both the public image and overlays.
---
Nitpick comments:
In `@tests/runs/conftest.py`:
- Around line 13-32: Update the docstring for the fixture force_pixi_argv to a
NumPy-style docstring that clearly documents its behavior and side effects:
describe that it mutates process environment variables (clears
SAMPLEWORKS_GRID_SEARCH_SCRIPT, SAMPLEWORKS_PIXI_PROJECT_DIR, unsets vars
starting with "SAMPLEWORKS_" and ending with "_PYTHON", and removes/sets
RUNTIME_PIXI, SAMPLEWORKS_ALLOW_RUNTIME_PIXI, SAMPLEWORKS_REQUIRE_PREBUILT_PIXI,
SAMPLEWORKS_SKIP_ENV_PREPARE, and sets SAMPLEWORKS_FORCE_PIXI="1") and that it
overwrites runner.WORKSPACE_GRID_SEARCH_SCRIPT to point at a tmp_path
missing-workspace run_grid_search.py; include parameter descriptions for
monkeypatch and tmp_path and a brief “Side effects” section listing the mutated
environment variables and the modified runner.WORKSPACE_GRID_SEARCH_SCRIPT.
🪄 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
Run ID: 759929c6-8491-4c25-ac72-b807b62ea0fe
📒 Files selected for processing (3)
DockerfileDockerfile.asteratests/runs/conftest.py
Summary\n- set HOME/XDG/SHELL in the public Sampleworks runtime image\n- create /home/dev state directories and make /home/dev the image WORKDIR\n- explicitly keep the Astera overlay WORKDIR aligned with ACTL workspace images\n\n## Testing\n- Not run locally: Docker image build requires checkpoint/image inputs not available in this environment.\n\n## Acceptance notes\n- After publish, verify
actl pod up --image sampleworksreportspwd=/home/devandHOME=/home/dev.\n- Once the image ships, the temporaryhomePath: /home/devcatalog override can be dropped from asteractl.Summary by CodeRabbit
Chores
Tests