use slimmer image for init container - #247
Conversation
|
I had claude run 3 tests locally using kind, and here are the results: |
|
Ran a few more tests... |
|
More test results after code changes |
|
is this also aimed for the next chart release? |
|
yes, just needs review by @bluestreak01 when he gets a moment |
|
Hi @sklarsa 👋 — here's a critical review of this PR. I validated the rewritten script end-to-end on a real Verdict: solid, low-risk change with a sound goal. Approve after considering one minor behavioral regression. Strengths
Issues (by severity)1. Low — broken hidden symlinks are silently left behind (behavioral regression). Very unlikely inside a QuestDB data dir, but easy to fix: for item in ./.[!.]* ./..?* ; do
[ -e "$item" ] || [ -L "$item" ] || continue
mv "$item" "$DEST_DIR/"
done2. Low/style — 3. Style — the main 4. Nit — no automated test. This is a data-relocating script with no CI coverage; the change rests on manual verification. A tiny shell test (or a 5. Nit — Bottom lineFunctionally correct and a net improvement in robustness and image size. None of the issues are blockers — I'd recommend just the one-line |
- Restore parity with the old bash script for broken hidden symlinks: `[ -e ]` follows the link so a broken symlink was silently skipped. Add `|| [ -L ]` so broken symlinks (hidden and visible) are migrated. - Replace `[ ... ] && continue` (fragile under `set -e`) with a plain `if` in the visible-files loop. - Add the same null-glob guard to the visible-files loop that the hidden loop already had, so the two loops behave consistently. - Document the intentional unquoted `$MARKER` glob with a shellcheck disable directive; `shellcheck -s sh` is now clean. Verified end-to-end under dash (POSIX sh) across normal, existing-db, broken visible/hidden symlink, and no-marker cases; helm lint passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addressed review feedback + re-validated on kindPushed Changes in this push
Test matrix — all pass
(Test 5 "empty volume" is identical to Test 1.) Evidence for the symlink fix (Test 6)Seeded volume root: After migration, Test 3 data survival (host
|
`ls $MARKER` lists a directory's *contents*, so if the tables.d.* glob matched an empty directory the guard read "not found" and skipped the migration entirely. `ls -d` lists each match's own name instead, so any match (file, empty dir, or non-empty dir) correctly registers as found. Pre-existing edge (the old bash script had the same `ls $MARKER`); the marker is normally a file, but this closes the hole cheaply on a line already touched by this PR. Verified under dash and on a real alpine:3.23 busybox init container in kind: empty-directory marker now migrates (realtable.d relocated into db/ with contents intact); file/non-empty-dir markers and the no-marker "Nothing to move" path are unchanged. shellcheck -s sh clean; helm lint passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ready for review — summary of changes since the first reviewThree commits address the review feedback, each verified end-to-end on a local kind cluster (
Verification (all green)
Known follow-up (not in scope here)The migration is not crash-resumable — if the init container is killed mid-run it can strand data on re-run. This is pre-existing (the old This PR is a faithful, behavior-preserving POSIX/alpine port and is ready for review. |
This PR swaps the data migration init container to a much smaller
alpinedistro to reduce the potential attack surface.Since
bashis not installed, we needed to update the migration script to make it POSIX-compliant.