Check for Docker-Prep updates when launching host prep - #57
Conversation
Have Installers/Docker-Prep.sh compare the local pin to the newest v* tag (or main) before download, offer to update the pin and checksums, then run the selected revision. Co-authored-by: Michael <Narehood@users.noreply.github.com>
📝 WalkthroughWalkthroughDocker-Prep now resolves and applies newer revision pins, persists accepted pins, refreshes checksums, and uses the effective revision for installation. New tests validate tag selection and launcher updates. Documentation and security checks reflect the new behavior. ChangesDocker-Prep pinning
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/docker-prep-update.sh (1)
41-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis block re-implements the production selection logic, so it cannot detect a regression in
resolve_latest_docker_prep.Lines 42-44 copy the pipeline from
Installers/Docker-Prep.sh. If the production pipeline changes or breaks, this test still passes. The test validatessort -Vandawk, not the function.Extract the selection into a helper that accepts the
ls-remoteoutput, then call that helper from both the function and the test. For example, addselect_latest_tag()inInstallers/Docker-Prep.shthat reads the tag listing on stdin and setsLATEST_SHAandLATEST_VERSION, and haveresolve_latest_docker_preppipetags_outinto it.🤖 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/docker-prep-update.sh` around lines 41 - 52, Extract the tag-selection pipeline into a shared select_latest_tag helper in Installers/Docker-Prep.sh that reads ls-remote output from stdin and sets LATEST_SHA and LATEST_VERSION. Update resolve_latest_docker_prep to pipe tags_out into this helper, and update the test to invoke the same helper instead of reimplementing awk/sort logic while preserving the existing expected tag and SHA assertions.
🤖 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 `@Installers/Docker-Prep.sh`:
- Around line 85-92: Replace the truncating `cat "$temporary" > "$SCRIPT_PATH"`
operation in the script-update flow with an atomic rename of the temporary file
into place using `mv`. Keep the temporary file created in the script’s directory
so the rename remains atomic, and preserve the existing script mode when
replacing it.
- Around line 37-44: Update resolve_latest_docker_prep’s tag lookup to call git
ls-remote --tags without --refs, identify the latest version tag, and prefer its
peeled refs/tags/<tag>^{} commit SHA when available. Fall back to the unpeeled
refs/tags/<tag> SHA for lightweight tags, then retain the existing
refs/heads/main fallback when no usable tag is found.
---
Nitpick comments:
In `@tests/docker-prep-update.sh`:
- Around line 41-52: Extract the tag-selection pipeline into a shared
select_latest_tag helper in Installers/Docker-Prep.sh that reads ls-remote
output from stdin and sets LATEST_SHA and LATEST_VERSION. Update
resolve_latest_docker_prep to pipe tags_out into this helper, and update the
test to invoke the same helper instead of reimplementing awk/sort logic while
preserving the existing expected tag and SHA assertions.
🪄 Autofix
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: 299dfd04-0ce6-420e-bf69-565c24c8fca1
📒 Files selected for processing (5)
Installers/.checksums.sha256Installers/Docker-Prep.shREADME.mdtests/docker-prep-update.shtests/security-checks.sh
| tags_out=$(git ls-remote --tags --refs "$REPO_URL" 'refs/tags/v*' 2>/dev/null || true) | ||
| if [[ -n "$tags_out" ]]; then | ||
| latest_tag=$(printf '%s\n' "$tags_out" | awk '{print $2}' | sed 's#refs/tags/##' | sort -V | tail -n1) | ||
| if [[ -n "$latest_tag" ]]; then | ||
| LATEST_SHA=$(printf '%s\n' "$tags_out" | awk -v tag="refs/tags/${latest_tag}" '$2 == tag { print $1; exit }') | ||
| LATEST_VERSION="$latest_tag" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Show raw and --refs ls-remote output for the pinned upstream repository.
set -euo pipefail
url="https://github.com/Narehood/Docker-Prep.git"
echo "--- with --refs (peeled entries suppressed) ---"
git ls-remote --tags --refs "$url" 'refs/tags/v*' || true
echo "--- without --refs (peeled ^{} entries visible) ---"
git ls-remote --tags "$url" 'refs/tags/v*' || trueRepository: Narehood/VM-Setup
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files:"
git ls-files | sed -n '1,120p'
echo
echo "Find Docker-Prep.sh:"
fd -a 'Docker-Prep\.sh$' . || true
echo
echo "Search for REPO_URL and EFFECTIVE_REVISION:"
rg -n 'REPO_URL|EFFECTIVE_REVISION|LATEST_SHA|git rev-parse HEAD|Docker-Prep revision verification failed|ls-remote.*--tags|--refs|docker|Docker-Prep' . \
-g '!*.snap' -g '!node_modules/**' -g '!dist/**' -g '!build/**' | sed -n '1,240p'Repository: Narehood/VM-Setup
Length of output: 16579
🌐 Web query:
git ls-remote --refs tag annotated tag output object SHA tags entry
💡 Result:
When running git ls-remote, the output behavior for annotated tags differs from lightweight tags because annotated tags in Git are distinct objects containing metadata (tagger, date, message) that point to a target object (usually a commit) [1]. By default, git ls-remote displays two lines for every annotated tag to provide both the tag object itself and the object it points to [2][3]: 1. The first line shows the SHA of the annotated tag object itself (e.g., refs/tags/v1.0.0) [2][4]. 2. The second line, appended with ^{}, shows the SHA of the object that the tag points to (the "peeled" value) [2][5]. For example: d4ca2e3147b409459955613c152220f4db848ee1 refs/tags/v2.40.0 73876f4861cd3d187a4682290ab75c9dccadbc56 refs/tags/v2.40.0^{} If you only want to see the tag reference without the peeled object SHA, you can use the --refs flag [2][5]. This suppresses the output of the peeled tag (the ^{} line) and shows only the reference to the tag object itself [2][6]. Lightweight tags, which are simply direct pointers to a commit without an intermediary tag object, will only ever display a single line in the output regardless of whether --refs is used [1].
Citations:
- 1: https://git-scm.com/book/en/v2/Git-Internals-Git-References
- 2: https://git-scm.com/docs/git-ls-remote
- 3: https://git-scm.com/docs/git-ls-remote.html
- 4: Inconsistency with commit variant due to annotated tags in git spack/spack#51446
- 5: https://superuser.com/questions/1445823/what-does-mean-in-the-tags
- 6: https://github.com/git/git/blob/maint/builtin/ls-remote.c
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Docker-Prep.sh relevant sections ---"
sed -n '1,70p' Installers/Docker-Prep.sh | cat -n
echo "---"
sed -n '160,190p' Installers/Docker-Prep.sh | cat -n
echo
echo "--- sync-docker-prep-pin.sh relevant sections ---"
sed -n '1,75p' tools/sync-docker-prep-pin.sh | cat -n
echo
echo "--- test fixture relevant section ---"
sed -n '35,55p' tests/docker-prep-update.sh | cat -n
echo
echo "--- check existence of Narehood Docker-Prep upstream tags ---"
tmpdir="$(mktemp -d)"
mkdir -p "$tmpdir/pkg1" "$tmpdir/pkg2"
git init -q "$tmpdir/pkg1"
git -C "$tmpdir/pkg1" config user.email test@example.test
git -C "$tmpdir/pkg1" config user.name Test
echo light > "$tmpdir/pkg1/a.txt"
git -C "$tmpdir/pkg1" add a.txt
git -C "$tmpdir/pkg1" commit -q -m "add"
git -C "$tmpdir/pkg1" tag v1-light
echo ann > "$tmpdir/pkg1/a2.txt"
git -C "$tmpdir/pkg1" add a2.txt
git -C "$tmpdir/pkg1" commit -q -m "add ann"
git -C "$tmpdir/pkg1" tag -a -m "annotated" v1-annotated
git clone -q "$tmpdir/pkg1" "$tmpdir/pkg2"
git -C "$tmpdir/pkg2" clone -q --bare "$tmpdir/pkg1" "$tmpdir/pkg1.git"
echo "ls-remote --tags --refs:"
git ls-remote --tags --refs "$tmpdir/pkg1.git" 'refs/tags/v*' || true
echo "ls-remote --tags:"
git ls-remote --tags "$tmpdir/pkg1.git" 'refs/tags/v*' || true
python3 - <<'PY'
import subprocess
for tags_out in [
subprocess.check_output(["git","ls-remote","--tags","--refs","/tmp/pkg1.git","refs/tags/v*"], text=True),
subprocess.check_output(["git","ls-remote","--tags","/tmp/pkg1.git","refs/tags/v*"], text=True),
]:
print("--- parsed sha2 ---")
for line in tags_out.splitlines():
print(repr(line.split()[1]))
print("--- latest sha2 ---")
lines = [line for line in tags_out.splitlines() if line.strip()]
if lines:
print(repr(lines[-1].split()[1]))
PYRepository: Narehood/VM-Setup
Length of output: 9652
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verify Narehood Docker-Prep tag refs from GitHub API:"
python3 - <<'PY'
import json, urllib.request
repo = "Narehood/Docker-Prep"
url = f"https://api.github.com/repos/{repo}/git/ref/tags/v2.4.0"
print("url:", url)
try:
with urllib.request.urlopen(url, timeout=20) as r:
data = r.read()
info = json.loads(data)
print("status:", r.status)
print(json.dumps({k: info.get(k) for k in ["ref","object"]}, indent=2))
except Exception as e:
print("ERROR:", type(e).__name__, e)
PY
echo
echo "Fetch raw ls-remote output for Narehood Docker-Prep:"
git ls-remote --tags --refs https://github.com/Narehood/Docker-Prep.git 'refs/tags/v2.4.0' || true
echo "---"
git ls-remote --tags https://github.com/Narehood/Docker-Prep.git 'refs/tags/v2.4.0' || trueRepository: Narehood/VM-Setup
Length of output: 493
Prefer peeled tag commits before falling back to main.
With --refs, git ls-remote --tags can return only the annotated tag object SHA for refs/tags/v2.4.0, and no ^{} commit SHA. resolve_latest_docker_prep() then accepts that tag object SHA, later fetches it, and the checkout/re-parse step compares the commit against EFFECTIVE_REVISION, so the launch can print "Docker-Prep revision verification failed". Use git ls-remote --tags without --refs, select the latest refs/tags/<tag>^{} commit when available, and fall back to refs/tags/<tag> for lightweight tags before using refs/heads/main.
🤖 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 `@Installers/Docker-Prep.sh` around lines 37 - 44, Update
resolve_latest_docker_prep’s tag lookup to call git ls-remote --tags without
--refs, identify the latest version tag, and prefer its peeled
refs/tags/<tag>^{} commit SHA when available. Fall back to the unpeeled
refs/tags/<tag> SHA for lightweight tags, then retain the existing
refs/heads/main fallback when no usable tag is found.
| ' "$SCRIPT_PATH" > "$temporary"; then | ||
| rm -f -- "$temporary" | ||
| return 1 | ||
| fi | ||
|
|
||
| cat "$temporary" > "$SCRIPT_PATH" | ||
| rm -f -- "$temporary" | ||
| return 0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Do not truncate and rewrite the script file that is currently executing.
cat "$temporary" > "$SCRIPT_PATH" truncates and rewrites the same inode that Bash is currently reading. Bash reads a script incrementally and tracks a byte offset in the open file. REPO_VERSION changes length (for example unreleased to v1.10.0), so every byte after that line shifts. Bash then resumes at a stale offset and can execute a partial or duplicated line. The failure appears after the pin update, in the fetch and install path.
Replace the file by rename instead. mv swaps the directory entry, so the running shell keeps the original inode and offset.
🐛 Proposed fix: atomic rename and preserved mode
local sha="$1"
local version="$2"
local temporary
- temporary=$(mktemp)
+ temporary=$(mktemp -- "${SCRIPT_PATH}.XXXXXX")
if ! awk -v sha="$sha" -v version="$version" '
@@
' "$SCRIPT_PATH" > "$temporary"; then
rm -f -- "$temporary"
return 1
fi
- cat "$temporary" > "$SCRIPT_PATH"
- rm -f -- "$temporary"
+ chmod --reference="$SCRIPT_PATH" -- "$temporary" 2>/dev/null || chmod 0755 -- "$temporary"
+ if ! mv -f -- "$temporary" "$SCRIPT_PATH"; then
+ rm -f -- "$temporary"
+ return 1
+ fi
return 0Note: mktemp in the same directory keeps mv on one filesystem, so the rename stays atomic.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ' "$SCRIPT_PATH" > "$temporary"; then | |
| rm -f -- "$temporary" | |
| return 1 | |
| fi | |
| cat "$temporary" > "$SCRIPT_PATH" | |
| rm -f -- "$temporary" | |
| return 0 | |
| ' "$SCRIPT_PATH" > "$temporary"; then | |
| rm -f -- "$temporary" | |
| return 1 | |
| fi | |
| chmod --reference="$SCRIPT_PATH" -- "$temporary" 2>/dev/null || chmod 0755 -- "$temporary" | |
| if ! mv -f -- "$temporary" "$SCRIPT_PATH"; then | |
| rm -f -- "$temporary" | |
| return 1 | |
| fi | |
| return 0 |
🤖 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 `@Installers/Docker-Prep.sh` around lines 85 - 92, Replace the truncating `cat
"$temporary" > "$SCRIPT_PATH"` operation in the script-update flow with an
atomic rename of the temporary file into place using `mv`. Keep the temporary
file created in the script’s directory so the rename remains atomic, and
preserve the existing script mode when replacing it.
2 participants
Summary
Docker Host Prep always ran the pinned Docker-Prep revision and never checked whether a newer one existed. Inside Docker-Prep itself, ephemeral launches (from VM-Setup) intentionally disable self-update.
Change
When you choose Docker Host Preparation,
Installers/Docker-Prep.shnow:v*tags if present, otherwisemain)(y/N)REPO_REVISION/REPO_VERSION, regenerates.checksums.sha256, then fetches/runs that revisionThis unblocks hosts that are still on the old pin while Docker-Prep
mainhas moved ahead (and still works with the release-based pin sync workflow once releases exist).Summary by CodeRabbit
New Features
main, with optional pin updates.Documentation
Tests