Repository navigation
test(e2e): silence helm-template render in install-cozystack trace - #2615
Conversation
The pre-pull test captured each rendered chart into a shell variable (`kubeovn_manifests=$(helm template ...)`) so a render failure would trip `set -e` (bats has no `pipefail`, so piping helm into yq would mask a helm failure with yq's exit code). cozytest.sh runs every bats test under `set -x`, and bash expands captured command output into the trace as `+ var=<entire stdout>`. Two full helm renders (kubeovn + linstor) plus a `printf` re-expansion were ballooning the install step to ~40k log lines per CI run. Stage each render to a tmp file instead. The same exit-code semantics apply (a failed `helm template ... > file` still trips `set -e`), but the trace now only shows the commands themselves, not the rendered YAML. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses excessive CI log output caused by bash variable expansion during the 'Install Cozystack' e2e test. By switching from capturing large helm template outputs in variables to using temporary files, the PR prevents thousands of lines of YAML from being dumped into the CI trace while preserving necessary error-handling behaviors. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
github-actions
Bot
added
area/testing
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWrites each ChangesE2E Image Pre-pull Test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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 |
dosubot
Bot
added
the
area/ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the image pre-pulling logic in the E2E installation script to use temporary files instead of variable captures. This change prevents CI logs from ballooning when tracing is enabled and ensures that Helm template failures are correctly caught. Feedback suggests further improving the script by using a trap for file cleanup, optimizing the yq call by passing files directly, and addressing potential exit code masking in the shell pipeline.
| local kubeovn_yaml linstor_yaml | ||
| kubeovn_yaml=$(mktemp) | ||
| linstor_yaml=$(mktemp) | ||
| helm template packages/system/kubeovn > "$kubeovn_yaml" | ||
| helm template packages/system/linstor > "$linstor_yaml" | ||
| cat "$kubeovn_yaml" "$linstor_yaml" | yq -N ' | ||
| (..|select(has("containers"))|.containers[]|.image), | ||
| (..|select(has("initContainers"))|.initContainers[]|.image) | ||
| ' | hack/e2e-prepull-images.sh | ||
| rm -f "$kubeovn_yaml" "$linstor_yaml" |
There was a problem hiding this comment.
This refactoring improves the code in several ways:
- Cleanup: Using a
trapensures that temporary files are removed even if the test fails (each Bats test runs in its own subshell). - Optimization: Passing files directly to
yqavoids an unnecessarycatprocess. - Correctness: Note that while
helm templatefailures are now caught byset -e, theyqcommand is still in a pipeline where its exit code is masked because Bats does not enablepipefailby default. Ifyqfails and produces no output, the prepull script will exit with 0, and the test will pass without actually pre-pulling images. Consider addingset -o pipefailif you want to ensureyqerrors also trigger a test failure.
local kubeovn_yaml linstor_yaml
kubeovn_yaml=$(mktemp)
linstor_yaml=$(mktemp)
trap 'rm -f "$kubeovn_yaml" "$linstor_yaml"' EXIT
helm template packages/system/kubeovn > "$kubeovn_yaml"
helm template packages/system/linstor > "$linstor_yaml"
yq -N '
(..|select(has("containers"))|.containers[]|.image),
(..|select(has("initContainers"))|.initContainers[]|.image)
' "$kubeovn_yaml" "$linstor_yaml" | hack/e2e-prepull-images.sh
The previous form staged each helm-template render to a tmp file so a render failure would trip `set -e` outside the pipe. That avoided the `var=$(...)` trace-bloat trap, but yq still ran inside a pipe with pipefail off, so a yq failure (bad expression, schema drift) would be masked by the prepull script's exit code and the test would pass while silently pre-pulling nothing. Replace the tmp-file dance with a brace group + `set -o pipefail`: helm-template output flows through the pipe, never expanded into the `set -x` trace (verified under `hack/cozytest.sh`: 5 command lines, no YAML), and pipefail now surfaces a failure at any stage. Drops the mktemp/rm scaffolding and the `cat` subprocess. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
The prior commit added `set -o pipefail` to catch yq failures in the
prepull pipeline. CI failed at runtime: hack/cozytest.sh has a
`#!/bin/sh` shebang, /bin/sh is dash on Ubuntu CI, and dash rejects
`set -o pipefail` ("Illegal option").
Drop the pipe entirely. Each step is now its own command with stdin/
stdout redirection — set -e catches failures at the helm-template, yq,
or prepull stage without needing pipefail and without `var=$(...)`
captures that would balloon the `set -x` trace with rendered YAML.
Verified under dash in Ubuntu 24.04 by running cozytest.sh against the
test.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>
There was a problem hiding this comment.
LGTM — trace-bloat fix is correct and the file-staging approach is the right choice given the dash/pipefail constraint inside the e2e sandbox.
Business context: The "Install Cozystack into sandbox" CI step was emitting ~40k log lines because var=$(helm template ...) captures were expanded into the set -x trace under cozytest.sh. This PR keeps the same set-e safety property while keeping the rendered YAML out of the trace.
Verified
- The bloat reproduces as described.
hack/cozytest.shruns each test underset -eu -x(hack/cozytest.sh:36) withPS4='+ ', and the trace reader iterates line-by-line, so a multi-line trace line lands in the log as one timestamped output line per content line. Three expansions per render (two captures + one printf re-expansion) match the reported magnitude. - The pipefail rationale is correct. The e2e sandbox image is
ubuntu:22.04(packages/core/testing/images/e2e-sandbox/Dockerfile:1) andmake install-cozystackinvokescozytest.shviadocker exec ... sh -c(packages/core/testing/Makefile:37) — dash on 22.04 (0.5.11) has noset -o pipefail. - Safety property preserved: each of
helm template > file,yq ... > file, andprepull < fileis a standalone command, soset -eaborts on any failure without depending on pipefail. - Output semantics equivalent.
yq -N '<expr>' file1 file2processes both files and emits images one per line;hack/e2e-prepull-images.shsorts/uniqs and tolerates either order.
3 participants
What this PR does
Trims the
Install Cozystack into sandboxCI step from ~40k log lines back down to a normal size.The
Pre-pull platform imagesbats test was capturing each rendered chart into a shell variable so a render failure would tripset -e:hack/cozytest.shruns every bats test underset -x, and bash expands captured command output into the trace as+ var=<entire stdout>. The kubeovn and linstor renders are several thousand lines each, and theprintfline re-expands both variables, so the same YAML was being dumped to the log three times.This PR stages each render to a tmp file instead:
The original safety property is preserved —
helm template ... > fileis a simple command whose exit code still triggersset -eon failure, exactly likevar=$(helm template ...)did. The trace now shows the command line, not its rendered output.Release note
Summary by CodeRabbit