Uh oh!
There was an error while loading. Please reload this page.
refactor(e2e): single-source the k3d bring-up prelude (#541 review) - #550
Conversation
LukasWodka
commented
Aug 3, 2026
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f709559. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f709559. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
saqlainsyed007
left a comment
There was a problem hiding this comment.
Blocking: the extracted lib scripts/tests/lib/e2e-common.sh is never committed
The entire refactor pivots on a new shared file, but that file is not part of this commit. git show --stat f709559 lists only the 6 modified files and adds nothing new; git cat-file -e HEAD:scripts/tests/lib/e2e-common.sh → does not exist in HEAD. There is no e2e-common.sh anywhere in the tree. This looks like a missing git add of the newly-created lib.
Consequences — this is not behavior-preserving, it is a hard break of every call site:
- Runtime abort in all 4 scripts.
e2e-cluster.sh:26,e2e-proxy.sh:28,e2e-journey.sh:48,e2e-auto-upgrade.sh:51each dosource "$HERE/lib/e2e-common.sh". Under theset -euo pipefailat the top of each script, sourcing a missing file fails immediately (No such file or directory, non-zero) and the script exits before ever reachingcreate_cluster. Thee2e-cluster/e2e-proxy/e2e-journey/upgrade-e2eCI jobs all die at bring-up. Even if the source somehow continued,e2e_isolate_envande2e_install_prereqsare undefined → the isolation env (CLUSTER_NAME/USER/TRACEBLOC_NO_AUTOSTART) and the docker-check/umask/tool-installs are silently lost, so the seal-check/egress posture the #541 review depends on is gone. - Both shellcheck gates fail on a missing path.
standard-checks.yml:47andinstaller-tests.yaml:70,74now passscripts/tests/lib/e2e-common.shtoshellcheck; with the file absent, shellcheck errorsopenBinaryFile: does not exist, breaking the required Lint. This contradicts the PR's "shellcheck clean on all 5 files" / "CI exercises the refactored scripts" claims — those can only have passed against an uncommitted local file.
Fix:git add scripts/tests/lib/e2e-common.sh and push. I could not review the actual behavior-preservation of e2e_isolate_env / e2e_install_prereqs (dropped flags, ordering, env defaults, has docker guard, umask 022) because the file isn't in the PR — please re-request review once the lib is committed so the extracted bodies can be diffed against the former inline copies.
Verdict: REQUEST_CHANGES.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Saqlain flagged (PR #541, thread on e2e-seal-check.sh:36) that the bring-up prelude is copy-pasted near-verbatim across scripts/tests/e2e-*.sh, and multiple Bugbot rounds have had to edit every copy in lockstep. Extract the two truly-identical, drift-prone blocks into scripts/tests/lib/e2e-common.sh: - e2e_isolate_env <name> — USER + CLUSTER_NAME default + TRACEBLOC_NO_AUTOSTART - e2e_install_prereqs — has docker + umask + install_{kubectl,k3d,helm} e2e-cluster / e2e-proxy / e2e-journey / e2e-auto-upgrade now source the lib and call these; each keeps its own CLUSTER_NAME default (passed as the arg) and its distinct logic. auto-upgrade keeps its extra `has jq` guard before the call. Deliberately NOT unified (would change behavior): the sub-lib `source` set (proxy/journey source 3 libs, not preflight — a pre-existing inconsistency, flagged not fixed), the cleanup/trap bodies (each reaps its own squid/work dirs), and CHART_DIR (only the chart-installing scripts). e2e-seal-check.sh (open on #541) adopts the lib as a fast-follow once both land — kept non-stacked. Added the lib to both shellcheck gates (installer-tests + the required standard-checks Lint). shellcheck --severity=error/warning clean; bash -n ok. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
f709559 to
a2b2be3CompareLukasWodka
commented
Aug 3, 2026
Force-pushed the fix: the new |
saqlainsyed007
left a comment
There was a problem hiding this comment.
Re-review of a2b2be3 (prior REQUEST_CHANGES was review 4844149284 at f709559).
Prior blocker RESOLVED. The one root-cause finding — the extracted shared file scripts/tests/lib/e2e-common.sh was never git added — is fixed: the file is now committed and tracked in the tree at this SHA, appears in the PR file list (ADDED, +51), and .gitignore now carries !scripts/tests/lib/ (mirroring the existing !scripts/lib/) so the dir is no longer swallowed by the lib/ ignore. Every source "$HERE/lib/e2e-common.sh" now resolves, so set -euo pipefail no longer aborts the four callers, and both shellcheck lists reference a file that exists.
Behavior preservation now verifiable — and faithful.
e2e_isolate_env <name>reproduces the old inline block exactly (export USER="${USER:-$(id -un)}",export CLUSTER_NAME="${CLUSTER_NAME:-$1}",export TRACEBLOC_NO_AUTOSTART=1). Each caller passes its own former default — e2e-clustertbe2e, e2e-journeytbe2e-journey, e2e-proxytbproxy, e2e-auto-upgradetbupg— and env override is preserved.e2e_install_prereqsreproduces the old block byte-for-byte:has docker || error,umask 022,install_kubectl/install_k3d/install_helm.- Sourcing/ordering is correct in all four: the lib is sourced first (functions only, no side effects, no
set -e— right for a sourced lib),e2e_isolate_envruns, then common.sh + setup-linux.sh + cluster.sh are sourced, ande2e_install_prereqsis called only afterward, sohas/error/install_*resolve at call time. - Correctly NOT unified:
preflight.shis still sourced only by e2e-cluster + e2e-auto-upgrade (the pre-existing proxy/journey inconsistency is flagged in the lib header, not silently "fixed"); each script keeps its owncleanup/trap cleanup EXIT; CHART_DIR stays local to auto-upgrade. - e2e-auto-upgrade keeps its extra
has jq || errorguard. (Minor, non-blocking: it now runs just before the docker check insidee2e_install_prereqsrather than just after it — both are fail-fast guards, so behavior is unchanged.) - e2e-seal-check.sh is deliberately untouched (fast-follow per the description) and still carries its own inline prelude, so nothing is half-migrated or double-brought-up. The #541 seal-check/egress posture is unaffected.
Verification I ran at this SHA:git cat-file -e HEAD:scripts/tests/lib/e2e-common.sh (exists); bash -n clean on the lib + all four callers; shellcheck --severity=errorclean across the lib, the four callers, and e2e-seal-check (the required standard-checks Lint gate), and --severity=warning clean too. Both workflow shellcheck lists now include scripts/tests/lib/e2e-common.sh.
No blocking issues and no behavior regressions. Approving.
Uh oh!
There was an error while loading. Please reload this page.
…ollow) (#566) Promised on the #541 review threads, now that #550 (the shared lib) and #541 (the seal-check script) are both on develop: - e2e-seal-check.sh now sources scripts/tests/lib/e2e-common.sh and uses e2e_isolate_env / e2e_install_prereqs, matching the other e2e-*.sh (drops the inlined isolation-env + install block). Keeps its own NS=$CLUSTER_NAME and local fail() for its assertions. - Harden the positive control (Saqlain nit): pin networkPolicy.training. enforcementProbeHost to a single $HOST var the install passes AND the positive control targets, so the probe and the control can never drift onto different hosts (was: HOST hardcoded while the probe used the chart default). shellcheck --severity=error/warning clean; helm template renders the probe Job with HOST=1.1.1.1 pinned. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 3, 2026
/fr-pass |

Single-source the e2e k3d bring-up prelude
Addresses @saqlainsyed007's review on #541 (thread on
scripts/tests/e2e-seal-check.sh:36): the bring-up prelude was copy-pasted near-verbatim acrossscripts/tests/e2e-*.sh, and multiple Bugbot rounds had to edit every copy in lockstep or they'd drift.What's extracted →
scripts/tests/lib/e2e-common.shTwo functions — the parts that were byte-identical and drift-prone:
e2e_isolate_env <name>—USER+CLUSTER_NAMEdefault +TRACEBLOC_NO_AUTOSTART(the distinct default is the arg, still env-overridable).e2e_install_prereqs—has docker+umask 022+install_{kubectl,k3d,helm}(the sequence the Bugbot rounds kept changing).e2e-cluster/e2e-proxy/e2e-journey/e2e-auto-upgradenow source the lib and call these; each keeps its ownCLUSTER_NAMEand test logic.e2e-auto-upgradekeeps its extrahas jqguard right before the call.Deliberately not unified (unifying would change behavior)
sourceset —e2e-proxy/e2e-journeysourcecommon+setup-linux+clusteronly;e2e-cluster/e2e-auto-upgradealso sourcepreflight(which has top-levelPF_*side effects).create_cluster— which uses_pf_recheck_runtime_mem, defined only inpreflight.sh— without sourcing it. That's a pre-existing inconsistency worth a separate look; I did not change it here.cleanup/trapbodies — each reaps its own extra resources (a squid container, work dirs) beyond the k3d cluster.CHART_DIR— only the chart-installing scripts set it.Scope note
e2e-seal-check.shlives on the still-open #541 branch, notdevelop, so it's not touched here (keeps this PR non-stacked). It adopts the lib as a fast-follow once both land.Verified
installer-tests.yamland the requiredstandard-checks.ymlLint.shellcheck --severity=error(the gate) and--severity=warningclean on all 5 files;bash -nok.e2e-cluster/e2e-proxy/e2e-journey/upgrade-e2eCI jobs exercise the refactored scripts on this PR.Targets
develop.🤖 Generated with Claude Code
Note
Low Risk
Test-harness refactor with no production installer behavior change; existing e2e CI jobs exercise the updated scripts.
Overview
Centralizes the duplicated k3d e2e bring-up prelude in
scripts/tests/lib/e2e-common.sh:e2e_isolate_envsetsUSER, a per-script defaultCLUSTER_NAME, andTRACEBLOC_NO_AUTOSTART;e2e_install_prereqschecks Docker, setsumask, and runsinstall_kubectl/install_k3d/install_helm.e2e-cluster,e2e-proxy,e2e-journey, ande2e-auto-upgradenow source that lib instead of inlining the same blocks (each still passes its own cluster name; auto-upgrade keeps its separatejqcheck).CI Lint and installer-tests shellcheck steps include the new file.
.gitignoreadds!scripts/tests/lib/so the test lib is not swallowed by the existinglib/ignore rule.Reviewed by Cursor Bugbot for commit a2b2be3. Bugbot is set up for automated code reviews on this repo. Configure here.