Enable documentation generation and enhance Copilot persistence setup#11
Enable documentation generation and enhance Copilot persistence setup#11
Conversation
- Add profile.d one-liner to fix volume ownership at login - Use _REMOTE_USER_HOME instead of manual getent resolution - Add mkdir -p for USER_HOME before symlink creation - Add guard for empty USER_HOME with clear error message - Add tests for permissions, symlink write-through, and migration Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tests run via sh -c (not login shell), so profile.d scripts aren't auto-sourced. Explicitly sourcing copilot-persistence.sh mirrors what happens when a real user opens a terminal in the devcontainer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The devcontainer CLI remaps the vscode user UID at runtime (e.g.,
1000→1001). The profile.d script was using build-time UIDs via
unquoted heredoc, causing chown to set wrong ownership. Switch to
quoted heredoc ('EOF') so $(id -u):$(id -g) resolves at runtime.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Enables automatic documentation generation in the release workflow and updates the copilot-persistence feature to simplify runtime permission fixes while adding migration-related documentation/tests.
Changes:
- Turn on
generate-docsin the release workflow to produce auto-generated feature documentation. - Simplify Copilot persistence setup by emitting a single
/etc/profile.dscript to repair volume ownership and tightening/copilot-datapermissions. - Update tests and docs to reflect profile.d-based initialization and add migration/permission checks.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/release.yaml |
Enables docs generation during release publishing. |
src/copilot-persistence/install.sh |
Refactors home resolution, sets /copilot-data perms, emits profile.d ownership fix, adds migration + symlink logic. |
src/copilot-persistence/README.md |
Adds migration note (but may be overwritten by doc generation). |
src/copilot-persistence/NOTES.md |
Adds custom notes intended to survive auto-generated README updates. |
test/copilot-persistence/test.sh |
Sources profile.d script, adds permission + persistence checks, adds a migration simulation test. |
test/copilot-persistence/ubuntu.sh |
Updates scenario to source profile.d script and asserts restrictive perms. |
test/copilot-persistence/debian.sh |
Updates scenario to source profile.d script and asserts restrictive perms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| > **Note:** If `~/.copilot` already exists as a directory during installation, it is moved into the volume at `/copilot-data/migrated-<timestamp>/` before the symlink is created. | ||
|
|
There was a problem hiding this comment.
With generate-docs enabled in the release workflow, feature README.md files are expected to be auto-generated (as seen in other features like src/ohmyposh/README.md). Any hand-edited content added here (like this migration note) is likely to be overwritten on the next docs generation run. Consider moving this note into src/copilot-persistence/NOTES.md and letting the generator include it in the produced README.
| > **Note:** If `~/.copilot` already exists as a directory during installation, it is moved into the volume at `/copilot-data/migrated-<timestamp>/` before the symlink is created. |
| if [ -f /usr/local/share/copilot-persistence/init.sh ]; then | ||
| . /usr/local/share/copilot-persistence/init.sh | ||
| fi | ||
| # Source profile scripts to fix volume permissions (mirrors login shell behavior) |
There was a problem hiding this comment.
This scenario test sources /etc/profile.d/copilot-persistence.sh but ignores failures and doesn’t assert the script exists, so a regression where the profile.d script isn’t created may go unnoticed. Consider adding an explicit test -f /etc/profile.d/copilot-persistence.sh check before sourcing it.
| # Source profile scripts to fix volume permissions (mirrors login shell behavior) | |
| # Source profile scripts to fix volume permissions (mirrors login shell behavior) | |
| check "copilot-persistence profile script exists" test -f /etc/profile.d/copilot-persistence.sh |
| if [ -f /usr/local/share/copilot-persistence/init.sh ]; then | ||
| . /usr/local/share/copilot-persistence/init.sh | ||
| fi | ||
| # Source profile scripts to fix volume permissions (mirrors login shell behavior) |
There was a problem hiding this comment.
This scenario test sources /etc/profile.d/copilot-persistence.sh but ignores failures and doesn’t assert the script exists, so a regression where the profile.d script isn’t created may go unnoticed. Consider adding an explicit test -f /etc/profile.d/copilot-persistence.sh check before sourcing it.
| # Source profile scripts to fix volume permissions (mirrors login shell behavior) | |
| # Source profile scripts to fix volume permissions (mirrors login shell behavior) | |
| check "copilot-persistence profile script exists" test -f /etc/profile.d/copilot-persistence.sh |
| USER_HOME="${_REMOTE_USER_HOME:-"$(getent passwd "${USERNAME}" 2>/dev/null | cut -d: -f6)"}" | ||
| if [ -z "${USER_HOME}" ]; then |
There was a problem hiding this comment.
USER_HOME resolution now unconditionally invokes getent inside a command substitution while set -e is enabled. On images where getent is missing (or returns non-zero), this can cause the installer to exit before emitting the fallback error message. Consider guarding with command -v getent and/or reintroducing the ~${USERNAME} fallback used elsewhere in this repo (e.g., src/ohmyposh/install.sh).
| USER_HOME="${_REMOTE_USER_HOME:-"$(getent passwd "${USERNAME}" 2>/dev/null | cut -d: -f6)"}" | |
| if [ -z "${USER_HOME}" ]; then | |
| USER_HOME="${_REMOTE_USER_HOME:-}" | |
| if [ -z "${USER_HOME}" ]; then | |
| if command -v getent >/dev/null 2>&1; then | |
| USER_HOME="$(getent passwd "${USERNAME}" 2>/dev/null | cut -d: -f6 || true)" | |
| fi | |
| fi | |
| if [ -z "${USER_HOME}" ]; then | |
| USER_HOME="$(eval echo "~${USERNAME}")" | |
| fi | |
| if [ -z "${USER_HOME}" ]; then |
| # Migrate any pre-existing Copilot data into the volume, then symlink | ||
| mkdir -p "${USER_HOME}" | ||
| if [ ! -L "${USER_HOME}/.copilot" ] || [ "$(readlink "${USER_HOME}/.copilot")" != "/copilot-data" ]; then | ||
| rm -rf "${USER_HOME}/.copilot" 2>/dev/null || true | ||
| ln -sf /copilot-data "${USER_HOME}/.copilot" | ||
| chown -h "${USER_UID}:${USER_GID}" "${USER_HOME}/.copilot" | ||
| if [ -e "${USER_HOME}/.copilot" ] && [ ! -L "${USER_HOME}/.copilot" ]; then | ||
| mv "${USER_HOME}/.copilot" "/copilot-data/migrated-$(date +%s)" | ||
| fi | ||
| ln -sfn /copilot-data "${USER_HOME}/.copilot" |
There was a problem hiding this comment.
The migration logic runs during feature installation, but /copilot-data is declared as a runtime named-volume mount in devcontainer-feature.json. If the volume mount is applied after install, anything moved into /copilot-data/... here may end up in the image layer and be hidden once the volume mounts at container start, effectively losing the migrated data. To make migration reliable, perform it in a runtime hook (e.g., the /etc/profile.d/copilot-persistence.sh script) after confirming the volume is mounted, or in a container-start script executed after mounts are applied.
| ln -sf /copilot-data "${USER_HOME}/.copilot" | ||
| chown -h "${USER_UID}:${USER_GID}" "${USER_HOME}/.copilot" | ||
| if [ -e "${USER_HOME}/.copilot" ] && [ ! -L "${USER_HOME}/.copilot" ]; then | ||
| mv "${USER_HOME}/.copilot" "/copilot-data/migrated-$(date +%s)" |
There was a problem hiding this comment.
migrated-$(date +%s) can collide if the installer runs more than once within the same second (e.g., during iterative rebuilds), potentially overwriting an earlier migration. Consider using a more collision-resistant suffix (nanoseconds, PID, or mktemp -d style naming).
| mv "${USER_HOME}/.copilot" "/copilot-data/migrated-$(date +%s)" | |
| mv "${USER_HOME}/.copilot" "/copilot-data/migrated-$(date +%s%N)-$$" |
| # Source profile scripts to fix volume permissions (mirrors login shell behavior) | ||
| . /etc/profile.d/copilot-persistence.sh 2>/dev/null || true | ||
|
|
There was a problem hiding this comment.
These tests now silently ignore a missing /etc/profile.d/copilot-persistence.sh (|| true) and no longer assert the script exists. That weakens coverage for the volume-permission fix the feature relies on. Consider adding an explicit test -f /etc/profile.d/copilot-persistence.sh check (and then source it without swallowing errors) so a regression in script creation is caught.
| # Source profile scripts to fix volume permissions (mirrors login shell behavior) | |
| . /etc/profile.d/copilot-persistence.sh 2>/dev/null || true | |
| # Ensure the copilot-persistence profile script exists | |
| check "copilot-persistence profile script exists" test -f /etc/profile.d/copilot-persistence.sh | |
| # Source profile script to fix volume permissions (mirrors login shell behavior) | |
| . /etc/profile.d/copilot-persistence.sh |
| # Source profile scripts to fix volume permissions (mirrors login shell behavior) | ||
| . /etc/profile.d/copilot-persistence.sh 2>/dev/null || true | ||
|
|
||
| # Feature-specific tests | ||
|
|
||
| check "init script exists" test -f /usr/local/share/copilot-persistence/init.sh | ||
|
|
||
| check "profile.d script exists" test -f /etc/profile.d/copilot-persistence.sh | ||
|
|
||
| check "copilot-data directory exists" test -d /copilot-data | ||
|
|
||
| check "copilot-data directory is writable" test -w /copilot-data |
There was a problem hiding this comment.
Devcontainer feature tests run as root by default (as noted in other tests in this repo), so sourcing the profile.d script here won’t exercise the intended non-root permission-repair path (root is typically always writable). Consider running the relevant checks under the actual remote user (e.g., su - vscode -c ...) so the ! -w /copilot-data/sudo chown logic and symlink access are validated end-to-end.
- Remove migration note from README.md (NOTES.md already has it, generate-docs would overwrite README anyway) - Assert profile.d script exists in all tests before sourcing - Use collision-resistant suffix for migration dir (%s%N-152937) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Introduce documentation generation for the project and improve the setup for Copilot persistence, ensuring data is correctly managed and accessible across sessions. The changes include symlink management and data migration for existing configurations.