Speed up and clean up acceptance tests#3248
Open
st3penta wants to merge 12 commits intoconforma:mainfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCI caching/tool installs added; per-scenario file-backed logs and verbose/format flags introduced; testcontainers container handles persisted and terminated for non-persisted runs; kind start now computes input SHA, builds/pushes CLI and bundle images with cache marker and applies resources with parallel namespace-scoped application. Changes
Sequence DiagramsequenceDiagram
participant TestRunner as Test Runner
participant Kind as Kind Cluster Manager
participant LocalBuild as Local Builder
participant Podman as Podman
participant Registry as Image Registry
TestRunner->>Kind: Start cluster setup
Kind->>Kind: ensure RBAC, applyResources(...)
Kind->>Kind: waitForDeployment("image-registry")
par concurrent builds & waits
Kind->>LocalBuild: computeSourceHash()
LocalBuild->>LocalBuild: check cache marker (/tmp/...hash)
alt cache miss
LocalBuild->>LocalBuild: go build ec (and kubectl if missing)
LocalBuild->>Podman: podman build (acceptance.Dockerfile, build-args)
Podman->>Registry: podman push (--tls-verify=false)
Podman->>LocalBuild: write cache marker (on success)
end
Kind->>LocalBuild: buildTaskBundleImage() (concurrent via errgroup)
LocalBuild->>Registry: push bundle images
Kind->>Kind: waitForAvailableDeploymentsIn(...,"tekton-pipelines")
end
Kind->>TestRunner: cluster ready / return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
acceptance/conftest/conftest.go (1)
95-115:⚠️ Potential issue | 🟡 MinorDebug output is now suppressed for non-execution failures.
Previously the deferred block emitted stdout/stderr whenever the function returned an error; it now only fires when
cmd.Run()itself failed. Failures in the subsequentReadFile/diff path (e.g., conftest ran but produced unexpected content) will no longer surface the command output, which can make such failures harder to debug. If that tradeoff is intentional (quieter passing runs, per-scenario log files elsewhere), ignore; otherwise, consider gating on the outererror ontestenv.VerboseOutputinstead ofcmdErr.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/conftest/conftest.go` around lines 95 - 115, The deferred debug-print block only runs when cmdErr is non-nil and cmdErr is set by cmd.Run(), so errors that occur later (e.g., file reads/diff checks after cmd.Run()) no longer trigger stdout/stderr printing; change the gating so the defer prints when the function returns any non-nil err or when verbose output is enabled: capture the named return error (err) or check testenv.VerboseOutput(ctx) inside the defer (instead of only cmdErr) and use that to decide whether to print; refer to the defer func, cmdErr, cmd.Run(), and testenv.VerboseOutput to locate and modify the condition.acceptance/wiremock/wiremock.go (1)
288-336:⚠️ Potential issue | 🟠 MajorConsolidate After hooks: termination currently runs before unmatched-requests check.
Two
sc.Afterhooks are registered at lines 289 and 318. In godog v0.15.0, After hooks execute in LIFO order (last registered first), so the termination hook runs before the unmatched-requests check. This terminates the WireMock container beforeUnmatchedRequests()can call its admin API, causing the check to fail.Consolidate both into a single hook, running the unmatched-requests check before termination.
Suggested consolidation
- sc.After(func(ctx context.Context, finished *godog.Scenario, scenarioErr error) (context.Context, error) { - if !IsRunning(ctx) { - return ctx, nil - } - - w, err := wiremockFrom(ctx) - if err != nil { - // wiremock wasn't launched, we don't need to proceed - return ctx, err - } - - unmatched, err := w.UnmatchedRequests() - if err != nil { - return ctx, err - } - - if len(unmatched) == 0 { - return ctx, nil - } - - logger, ctx := log.LoggerFor(ctx) - logger.Log("Found unmatched WireMock requests:") - for i, u := range unmatched { - logger.Logf("[%d]: %s", i, u) - } - - return ctx, nil - }) - - sc.After(func(ctx context.Context, finished *godog.Scenario, scenarioErr error) (context.Context, error) { - if testenv.Persisted(ctx) { - return ctx, nil - } - - if !testenv.HasState[wiremockState](ctx) { - return ctx, nil - } - - state := testenv.FetchState[wiremockState](ctx) - if state.Container != nil { - if err := state.Container.Terminate(ctx); err != nil { - logger, _ := log.LoggerFor(ctx) - logger.Warnf("failed to terminate wiremock container: %v", err) - } - } - - return ctx, nil - }) + sc.After(func(ctx context.Context, finished *godog.Scenario, scenarioErr error) (context.Context, error) { + if !IsRunning(ctx) { + return ctx, nil + } + + if w, err := wiremockFrom(ctx); err == nil { + if unmatched, err := w.UnmatchedRequests(); err == nil && len(unmatched) > 0 { + logger, _ := log.LoggerFor(ctx) + logger.Log("Found unmatched WireMock requests:") + for i, u := range unmatched { + logger.Logf("[%d]: %s", i, u) + } + } + } + + if testenv.Persisted(ctx) { + return ctx, nil + } + + state := testenv.FetchState[wiremockState](ctx) + if state.Container != nil { + if err := state.Container.Terminate(ctx); err != nil { + logger, _ := log.LoggerFor(ctx) + logger.Warnf("failed to terminate wiremock container: %v", err) + } + } + + return ctx, nil + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/wiremock/wiremock.go` around lines 288 - 336, The two sc.After hooks in AddStepsTo run LIFO so the termination hook (which uses testenv.FetchState[wiremockState] and state.Container.Terminate) currently runs before the UnmatchedRequests check (wiremockFrom(ctx).UnmatchedRequests()), causing the admin API to be unreachable; consolidate them into a single sc.After hook that first calls wiremockFrom(ctx) and w.UnmatchedRequests() (logging as currently done) and only after processing unmatched requests performs the testenv.HasState[wiremockState] / testenv.FetchState[wiremockState] termination logic (including the existing logger.Warnf on Terminate error), preserving the same early returns and error handling.
🧹 Nitpick comments (5)
acceptance/registry/registry.go (1)
330-348: Terminate uses the scenario's context, which may already be cancelled.If godog cancels
ctxon scenario failure/timeout,state.Container.Terminate(ctx)can fail immediately and the container may leak (Ryuk still reaps it, but termination becomes best-effort). Consider usingcontext.WithTimeout(context.Background(), ...)for teardown to ensure clean shutdown independent of scenario cancellation. Same pattern likely applies to the analogous After hooks added inacceptance/git/git.goandacceptance/wiremock/wiremock.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/registry/registry.go` around lines 330 - 348, The After hook uses the scenario's ctx which may be cancelled; change the teardown to call state.Container.Terminate with a fresh context that has a timeout (e.g., context.WithTimeout(context.Background(), <reasonableDuration>)) so termination runs independent of scenario cancellation; create the cancel func and defer cancel, call state.Container.Terminate(newCtx) and log errors as before. Apply the same pattern to the equivalent After hooks referencing registryState/state.Container.Terminate in acceptance/git/git.go and acceptance/wiremock/wiremock.go.acceptance/kubernetes/kind/acceptance.Dockerfile (1)
22-22: Consider separatingmicrodnf upgradefrom install and cleaning caches.Chaining
upgradeandinstallworks, but running them as separate steps (and ending withmicrodnf clean all) is the idiomatic UBI pattern and produces slightly smaller images. Minor, non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/acceptance.Dockerfile` at line 22, Split the combined microdnf command into two RUN steps: first run "microdnf upgrade --assumeyes --nodocs --setopt=keepcache=0 --refresh" and then run "microdnf -y --nodocs --setopt=keepcache=0 install gzip jq ca-certificates", and finally add a "microdnf clean all" (or equivalent) at the end of the install step to clear caches; update the Dockerfile commands around the existing microdnf upgrade and install invocations so they run in separate layers and include the clean step to reduce image size.acceptance/log/log.go (1)
165-168: Panic on temp-file creation failure is harsh but acceptable; consider returning an error instead for testability.
panicterminates the entire test binary on a transient filesystem issue. Returning an error (and changing the signature) would let callers degrade gracefully or retry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/log/log.go` around lines 165 - 168, Replace the panic on os.CreateTemp failure with error propagation: change the enclosing function (the one that calls os.CreateTemp(fmt.Sprintf("scenario-%010d-*.log", id))) to return an error (and the temp file or nil) instead of panicking, return the wrapped error when os.CreateTemp fails, and update all callers to handle/propagate the error; specifically modify the function signature to include error return, replace panic(fmt.Sprintf(..., err)) with returning fmt.Errorf("failed to create scenario log file: %w", err) (or equivalent), and adjust callers/tests accordingly..github/workflows/checks-codecov.yaml (1)
128-139: Fragile grep parsing may pick the wrong version if multiple matching lines exist.
grep 'tektoncd/cli' tools/go.modandgrep 'k8s.io/kubernetes' tools/kubectl/go.modmatch any line (including transitive/indirect entries) containing the substring. Ifgo.modever grows additional matching entries,awk '{print $2}'on multi-line output will break silently. Prefergo list -m -modfile=... <module>or a more precise anchored regex.- TKN_VERSION=$(grep 'tektoncd/cli' tools/go.mod | awk '{print $2}' | sed 's/^v//') + TKN_VERSION=$(go list -m -modfile=tools/go.mod -f '{{.Version}}' github.com/tektoncd/cli | sed 's/^v//') ... - KUBECTL_VERSION=$(grep 'k8s.io/kubernetes' tools/kubectl/go.mod | grep -oE 'v[0-9]+\.[0-9]+\.[0-9]+') + KUBECTL_VERSION=$(go list -m -modfile=tools/kubectl/go.mod -f '{{.Version}}' k8s.io/kubernetes)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/checks-codecov.yaml around lines 128 - 139, The current TKN_VERSION and KUBECTL_VERSION extraction using grep (the lines that set TKN_VERSION=$(grep 'tektoncd/cli' tools/go.mod ...) and KUBECTL_VERSION=$(grep 'k8s.io/kubernetes' tools/kubectl/go.mod ...) ) is fragile; replace those grep/awk commands with precise module resolution using go list -m -modfile=... (e.g., run go list -m -modfile=tools/go.mod tektoncd/cli and go list -m -modfile=tools/kubectl/go.mod k8s.io/kubernetes) to get exact versions, assign their stdout to TKN_VERSION and KUBECTL_VERSION, strip the leading "v" from TKN_VERSION if needed, and keep the existing curl/tar and curl/chmod steps unchanged; add a simple check to fail early if the go list command returns empty.Makefile (1)
137-140: Minor: test_passed preserves exit status, but a failure ingo tool covdata textfmton line 139 will short-circuit before the final[ ... ]check.With
.SHELLFLAGS=-e -c, ifgo tool covdata textfmtexits non-zero (e.g., no coverage files produced after test failure), the recipe will fail with that error and you'll lose thetest_passedsignal. Consider making the covdata step tolerant of missing data or placing the final check before it.- cd acceptance && go test -timeout $(ACCEPTANCE_TIMEOUT) ./... && test_passed=1 || test_passed=0; \ - echo "[`date '+%H:%M:%S'`] Tests finished in $$((SECONDS/60))m$$((SECONDS%60))s"; \ - go tool covdata textfmt -i=$${GOCOVERDIR} -o="$(ROOT_DIR)/coverage-acceptance.out"; \ - [ "$$test_passed" = "1" ] + cd acceptance && go test -timeout $(ACCEPTANCE_TIMEOUT) ./... && test_passed=1 || test_passed=0; \ + echo "[`date '+%H:%M:%S'`] Tests finished in $$((SECONDS/60))m$$((SECONDS%60))s"; \ + go tool covdata textfmt -i=$${GOCOVERDIR} -o="$(ROOT_DIR)/coverage-acceptance.out" || true; \ + [ "$$test_passed" = "1" ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 137 - 140, The recipe currently sets test_passed based on the `go test` result but then runs `go tool covdata textfmt` which can abort the shell (because of `.SHELLFLAGS=-e -c`) and lose the `test_passed` signal; update the Makefile so the covdata step cannot fail the recipe (e.g., make the `go tool covdata textfmt -i=${GOCOVERDIR} -o="${ROOT_DIR}/coverage-acceptance.out"` invocation tolerant of missing data by appending a conditional fallback such as `|| true` or otherwise swallowing non-zero exits) or move the final `[ "$$test_passed" = "1" ]` check to run before the covdata line; reference the `test_passed` variable, the `go tool covdata textfmt` invocation, and the `GOCOVERDIR`/`ROOT_DIR` variables when applying the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acceptance/acceptance_test.go`:
- Around line 235-241: The current TestMain uses klog.LogToStderr(false) and
klog.SetOutput(io.Discard) unconditionally which hides all k8s client warnings;
change TestMain so klog is silenced only when tests are not verbose (use
testing.Verbose() or a -verbose flag): if not verbose, call
klog.LogToStderr(false) and klog.SetOutput(io.Discard); otherwise restore normal
output (e.g., klog.LogToStderr(true) and klog.SetOutput(os.Stderr)) or wire klog
into the per-scenario logger used by your tests; update imports accordingly and
keep references to TestMain, klog.LogToStderr, klog.SetOutput, io.Discard (and
os.Stderr/testing.Verbose) so the behavior is reversible for debugging.
In `@acceptance/kubernetes/kind/image.go`:
- Around line 98-112: The code currently copies any kubectl found via
exec.LookPath into kubectlBinary which can lead to client/server version drift;
change the logic in this block (symbols: kubectlBinary, exec.LookPath, copyFile,
kubectlBuildCmd, tools/kubectl/go.mod) to prefer building the pinned kubectl
from tools/kubectl/go.mod by default and only use the PATH binary when an
explicit opt-in (env var or config flag like USE_HOST_KUBECTL) is set; if opt-in
is allowed, detect the host kubectl version (invoke kubectl version --client
--short) and log that version before copying, or compare it to the pinned
version and warn/error on mismatch to make cache/CI differences traceable.
- Around line 54-66: In buildCliImage, the cache file path currently uses a
fixed /tmp/ec-cli-image-cache-<port>.hash which can collide across users and
repos; change the cacheFile construction in computeSourceHash logic to use a
user-scoped cache directory (os.UserCacheDir(), falling back to os.TempDir())
and include a repo-specific and user-unique component (e.g., the current working
directory or repo root hash and UID) plus k.registryPort to form the filename so
cache files are isolated per user/checkout; update references to cacheFile in
buildCliImage to use that new path.
---
Outside diff comments:
In `@acceptance/conftest/conftest.go`:
- Around line 95-115: The deferred debug-print block only runs when cmdErr is
non-nil and cmdErr is set by cmd.Run(), so errors that occur later (e.g., file
reads/diff checks after cmd.Run()) no longer trigger stdout/stderr printing;
change the gating so the defer prints when the function returns any non-nil err
or when verbose output is enabled: capture the named return error (err) or check
testenv.VerboseOutput(ctx) inside the defer (instead of only cmdErr) and use
that to decide whether to print; refer to the defer func, cmdErr, cmd.Run(), and
testenv.VerboseOutput to locate and modify the condition.
In `@acceptance/wiremock/wiremock.go`:
- Around line 288-336: The two sc.After hooks in AddStepsTo run LIFO so the
termination hook (which uses testenv.FetchState[wiremockState] and
state.Container.Terminate) currently runs before the UnmatchedRequests check
(wiremockFrom(ctx).UnmatchedRequests()), causing the admin API to be
unreachable; consolidate them into a single sc.After hook that first calls
wiremockFrom(ctx) and w.UnmatchedRequests() (logging as currently done) and only
after processing unmatched requests performs the testenv.HasState[wiremockState]
/ testenv.FetchState[wiremockState] termination logic (including the existing
logger.Warnf on Terminate error), preserving the same early returns and error
handling.
---
Nitpick comments:
In @.github/workflows/checks-codecov.yaml:
- Around line 128-139: The current TKN_VERSION and KUBECTL_VERSION extraction
using grep (the lines that set TKN_VERSION=$(grep 'tektoncd/cli' tools/go.mod
...) and KUBECTL_VERSION=$(grep 'k8s.io/kubernetes' tools/kubectl/go.mod ...) )
is fragile; replace those grep/awk commands with precise module resolution using
go list -m -modfile=... (e.g., run go list -m -modfile=tools/go.mod tektoncd/cli
and go list -m -modfile=tools/kubectl/go.mod k8s.io/kubernetes) to get exact
versions, assign their stdout to TKN_VERSION and KUBECTL_VERSION, strip the
leading "v" from TKN_VERSION if needed, and keep the existing curl/tar and
curl/chmod steps unchanged; add a simple check to fail early if the go list
command returns empty.
In `@acceptance/kubernetes/kind/acceptance.Dockerfile`:
- Line 22: Split the combined microdnf command into two RUN steps: first run
"microdnf upgrade --assumeyes --nodocs --setopt=keepcache=0 --refresh" and then
run "microdnf -y --nodocs --setopt=keepcache=0 install gzip jq ca-certificates",
and finally add a "microdnf clean all" (or equivalent) at the end of the install
step to clear caches; update the Dockerfile commands around the existing
microdnf upgrade and install invocations so they run in separate layers and
include the clean step to reduce image size.
In `@acceptance/log/log.go`:
- Around line 165-168: Replace the panic on os.CreateTemp failure with error
propagation: change the enclosing function (the one that calls
os.CreateTemp(fmt.Sprintf("scenario-%010d-*.log", id))) to return an error (and
the temp file or nil) instead of panicking, return the wrapped error when
os.CreateTemp fails, and update all callers to handle/propagate the error;
specifically modify the function signature to include error return, replace
panic(fmt.Sprintf(..., err)) with returning fmt.Errorf("failed to create
scenario log file: %w", err) (or equivalent), and adjust callers/tests
accordingly.
In `@acceptance/registry/registry.go`:
- Around line 330-348: The After hook uses the scenario's ctx which may be
cancelled; change the teardown to call state.Container.Terminate with a fresh
context that has a timeout (e.g., context.WithTimeout(context.Background(),
<reasonableDuration>)) so termination runs independent of scenario cancellation;
create the cancel func and defer cancel, call state.Container.Terminate(newCtx)
and log errors as before. Apply the same pattern to the equivalent After hooks
referencing registryState/state.Container.Terminate in acceptance/git/git.go and
acceptance/wiremock/wiremock.go.
In `@Makefile`:
- Around line 137-140: The recipe currently sets test_passed based on the `go
test` result but then runs `go tool covdata textfmt` which can abort the shell
(because of `.SHELLFLAGS=-e -c`) and lose the `test_passed` signal; update the
Makefile so the covdata step cannot fail the recipe (e.g., make the `go tool
covdata textfmt -i=${GOCOVERDIR} -o="${ROOT_DIR}/coverage-acceptance.out"`
invocation tolerant of missing data by appending a conditional fallback such as
`|| true` or otherwise swallowing non-zero exits) or move the final `[
"$$test_passed" = "1" ]` check to run before the covdata line; reference the
`test_passed` variable, the `go tool covdata textfmt` invocation, and the
`GOCOVERDIR`/`ROOT_DIR` variables when applying the fix.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 20300c2d-e7e2-4dbb-85de-0d9a7a817b75
⛔ Files ignored due to path filters (1)
features/__snapshots__/validate_input.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
.github/workflows/checks-codecov.yamlMakefileacceptance/acceptance_test.goacceptance/cli/cli.goacceptance/conftest/conftest.goacceptance/git/git.goacceptance/go.modacceptance/kubernetes/kind/acceptance.Dockerfileacceptance/kubernetes/kind/image.goacceptance/kubernetes/kind/kind.goacceptance/kubernetes/kind/kubernetes.goacceptance/log/log.goacceptance/log/log_test.goacceptance/registry/registry.goacceptance/testenv/testenv.goacceptance/wiremock/wiremock.gofeatures/validate_input.featurehack/ubi-base-image-bump.sh
st3penta
force-pushed
the
EC-1710-impl-v3
branch
from
4daaa9f to
b758c00
Compare
Contributor
Author
|
Went through the CodeRabbit review and addressed two findings:
For the rest of the CodeRabbit suggestions, here's why we're leaving them as-is:
|
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
|
There's a cve |
st3penta
force-pushed
the
EC-1710-impl-v3
branch
from
7b8e811 to
1c8a560
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
acceptance/git/git.go (1)
192-200:⚠️ Potential issue | 🟠 MajorHandle partial startup in teardown.
state.Containeris set beforeHostAndPort/RepositoriesDir, but theAfterhook exits on!state.Up(). If startup fails after the container is created but before those fields are populated, the scenario leaks the git container and temp directory. Clean up based on the individual resources instead ofUp()here.♻️ Suggested fix
- if !state.Up() { - return ctx, nil - } - if state.Container != nil { if err := state.Container.Terminate(ctx); err != nil { logger, _ := log.LoggerFor(ctx) logger.Warnf("failed to terminate git container: %v", err) } } - os.RemoveAll(state.RepositoriesDir) + if state.RepositoriesDir != "" { + _ = os.RemoveAll(state.RepositoriesDir) + }As per coding guidelines, focus on major issues impacting performance, readability, maintainability and security.
Also applies to: 320-342
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/git/git.go` around lines 192 - 200, The teardown relies on state.Up() and leaks resources if the container is set before other fields; change teardown/After hook to clean each resource individually instead of checking Up(): check and stop/remove state.Container (if non-nil), remove the temp repositories dir when state.RepositoriesDir is non-empty, and only then clear HostAndPort; alternatively, delay assigning state.Container until after HostAndPort and RepositoriesDir are populated. Update the After hook logic that references state.Up() to instead inspect state.Container, state.RepositoriesDir, and HostAndPort to perform per-resource cleanup safely (also apply same change for the similar block around the 320-342 region).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/checks-codecov.yaml:
- Around line 128-139: The workflow installs tkn and kubectl by piping downloads
directly into /usr/local/bin (see the TKN_VERSION/TKN install step and
KUBECTL_VERSION/kubectl install step); modify each step to download the binary
and its corresponding checksum or signature file first, verify the
checksum/signature (e.g., use curl to fetch
tkn_${TKN_VERSION}_Linux_x86_64.tar.gz and its .sha256/.sha512 or a GitHub
release signature, verify with sha256sum -c or gpg --verify, and similarly fetch
kubectl and kubectl.sha256 from dl.k8s.io or the Kubernetes release artifacts
and compare before running sudo tar/chmod), and only move the verified binary
into /usr/local/bin; ensure failure on mismatch so unverified artifacts are
never installed.
In `@acceptance/kubernetes/kind/image.go`:
- Around line 55-66: The cache key currently computed by computeSourceHash
misses non-file build inputs (the derived version string injected via ldflags
and the host kubectl binary copied when present), causing stale cache hits;
update the cache/hash logic so computeSourceHash (or the code that calls it
before checking cacheFile) incorporates the resolved version string used for
ldflags and, if kubectl is found in PATH, a checksum (e.g., sha256) or mtime of
that kubectl binary into the hash input before writing/reading cacheFile (which
still may use k.registryPort in its name); ensure the same combined inputs are
used both to generate currentHash and to validate the cache hit so changes to
version or host kubectl force a rebuild of the CLI image.
In `@acceptance/kubernetes/kind/kind.go`:
- Around line 348-351: The code currently calls
k.mapper.RESTMapping(obj.GroupVersionKind().GroupKind()) during parse/classify
which fails for CRD instances because the mapper was built before CRDs were
applied; change logic to defer calling RESTMapping for objects whose GVK may
come from CRDs until after cluster-scoped resources are applied, by classifying
resources first, applying CRDs/cluster-scoped resources, then refresh the
discovery/RESTMapper (rebuild k.mapper) and resolve mappings for remaining
objects (the RESTMapping call and mapping variable resolution). Ensure
functions/methods referencing k.mapper, RESTMapping, and any parse/classify flow
are updated to postpone resolution and to call a discovery refresh method before
retrying RESTMapping.
In `@Makefile`:
- Around line 123-140: The acceptance target currently chains $(MAKE) build
E2E_INSTRUMENTATION=true with the test invocation using && which allows a failed
build to fall through under the current shell semantics; change the shell flow
around the build step in the acceptance recipe so a non‑zero exit from the build
immediately aborts the recipe (for example, run the build as its own command and
check its exit status or add an explicit || { echo "..."; exit 1; } after the
$(MAKE) build invocation) so tests only run when the build succeeds and
test_passed remains accurate.
---
Outside diff comments:
In `@acceptance/git/git.go`:
- Around line 192-200: The teardown relies on state.Up() and leaks resources if
the container is set before other fields; change teardown/After hook to clean
each resource individually instead of checking Up(): check and stop/remove
state.Container (if non-nil), remove the temp repositories dir when
state.RepositoriesDir is non-empty, and only then clear HostAndPort;
alternatively, delay assigning state.Container until after HostAndPort and
RepositoriesDir are populated. Update the After hook logic that references
state.Up() to instead inspect state.Container, state.RepositoriesDir, and
HostAndPort to perform per-resource cleanup safely (also apply same change for
the similar block around the 320-342 region).
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 064cd0c8-56df-49e9-8636-a1f394ba9afc
⛔ Files ignored due to path filters (1)
features/__snapshots__/validate_input.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
.github/workflows/checks-codecov.yamlMakefileacceptance/acceptance_test.goacceptance/cli/cli.goacceptance/conftest/conftest.goacceptance/git/git.goacceptance/go.modacceptance/kubernetes/kind/acceptance.Dockerfileacceptance/kubernetes/kind/image.goacceptance/kubernetes/kind/kind.goacceptance/kubernetes/kind/kubernetes.goacceptance/log/log.goacceptance/log/log_test.goacceptance/registry/registry.goacceptance/testenv/testenv.goacceptance/wiremock/wiremock.gofeatures/validate_input.featurehack/ubi-base-image-bump.sh
✅ Files skipped from review due to trivial changes (3)
- acceptance/conftest/conftest.go
- features/validate_input.feature
- acceptance/kubernetes/kind/acceptance.Dockerfile
🚧 Files skipped from review as they are similar to previous changes (6)
- acceptance/kubernetes/kind/kubernetes.go
- acceptance/testenv/testenv.go
- hack/ubi-base-image-bump.sh
- acceptance/go.mod
- acceptance/cli/cli.go
- acceptance/wiremock/wiremock.go
st3penta
force-pushed
the
EC-1710-impl-v3
branch
from
1c8a560 to
2e12f5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acceptance/kubernetes/kind/image.go`:
- Around line 277-282: The current change overwrites the entire resource spec by
setting steps[i].ComputeResources = corev1.ResourceRequirements{}, which removes
memory requests/limits and any intentional step limits; instead modify only the
CPU entries: fetch steps[i].ComputeResources, remove/clear the "cpu" key from
both Requests and Limits (or set them to nil/zero) while preserving other keys
(e.g., "memory") and any existing Limits/Requests maps, then assign the modified
ResourceRequirements back to steps[i].ComputeResources so only CPU
requests/limits are dropped.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c7fab6d-b191-41a6-ab03-efdc645f1e37
⛔ Files ignored due to path filters (1)
features/__snapshots__/validate_input.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
.github/workflows/checks-codecov.yamlMakefileacceptance/acceptance_test.goacceptance/cli/cli.goacceptance/conftest/conftest.goacceptance/git/git.goacceptance/go.modacceptance/kubernetes/kind/acceptance.Dockerfileacceptance/kubernetes/kind/image.goacceptance/kubernetes/kind/kind.goacceptance/kubernetes/kind/kubernetes.goacceptance/log/log.goacceptance/log/log_test.goacceptance/registry/registry.goacceptance/testenv/testenv.goacceptance/wiremock/wiremock.gofeatures/validate_input.featurehack/ubi-base-image-bump.sh
✅ Files skipped from review due to trivial changes (4)
- acceptance/conftest/conftest.go
- acceptance/testenv/testenv.go
- acceptance/kubernetes/kind/acceptance.Dockerfile
- features/validate_input.feature
🚧 Files skipped from review as they are similar to previous changes (4)
- acceptance/kubernetes/kind/kubernetes.go
- hack/ubi-base-image-bump.sh
- acceptance/go.mod
- acceptance/log/log_test.go
All 285 containers accumulated during the full test run with zero cleanup, wasting memory and adding Docker daemon overhead. Each container type (registry, git, wiremock) now stores the testcontainers.Container reference and calls Terminate(ctx) in the sc.After hook, respecting the persist flag. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ref: https://issues.redhat.com/browse/EC-1710
Each scenario now writes to its own temp file, which eliminates log interleaving across parallel goroutines. Remove shouldSuppress(), which incorrectly filtered Error-level messages, and remove /dev/tty writes that failed silently in CI. Replace os.Exit(1) with t.Fatalf() so TestMain cleanup runs. Print failed scenario log file paths in the test summary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ref: https://issues.redhat.com/browse/EC-1710
Instead of running make push-image (full multi-stage Dockerfile that compiles Go inside the container), build the ec and kubectl binaries locally with the host Go cache, then inject them into a minimal ubi-minimal base image via acceptance.Dockerfile. The old approach compiled Go twice: once on the host, once in the container. Skipping the in-container build significantly reduces CI build time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ref: https://issues.redhat.com/browse/EC-1710
st3penta
force-pushed
the
EC-1710-impl-v3
branch
from
2e12f5d to
ccec8aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
acceptance/kubernetes/kind/image.go (1)
279-282:⚠️ Potential issue | 🟠 MajorOnly drop the CPU request here.
ResourceRequirements{}also removes memory requests/limits and any intentional step limits. That changes task behavior more than the scheduling fix requires.As per coding guidelines, focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.Suggested change
- // Strip resource requests to avoid scheduling waterfall in acceptance tests. - // Each TaskRun pod requests 2600m CPU, limiting concurrent pods on the Kind - // node and causing scheduling delays up to 5 minutes. - steps[i].ComputeResources = corev1.ResourceRequirements{} + // Drop only CPU requests to avoid scheduling waterfall while preserving + // memory sizing and any explicit limits. + if steps[i].ComputeResources.Requests != nil { + delete(steps[i].ComputeResources.Requests, corev1.ResourceCPU) + if len(steps[i].ComputeResources.Requests) == 0 { + steps[i].ComputeResources.Requests = nil + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/image.go` around lines 279 - 282, Instead of overwriting steps[i].ComputeResources with an empty corev1.ResourceRequirements{}, remove only the CPU request entry so memory requests/limits and other settings remain intact: locate the code that assigns steps[i].ComputeResources = corev1.ResourceRequirements{} and change it to delete the corev1.ResourceCPU key from steps[i].ComputeResources.Requests (and leave Limits and other Request entries untouched), ensuring you check for a nil Requests map before deleting so you don't panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acceptance/kubernetes/kind/image.go`:
- Around line 59-60: Replace direct fmt.Print*/fmt.Println calls in
acceptance/kubernetes/kind/image.go with the scenario logger obtained via
log.LoggerFor(ctx); e.g., at the top of the function create a local logger :=
log.LoggerFor(ctx) and change fmt.Printf/fmt.Println/fmt.Printf calls to
logger.Infof or logger.Warnf as appropriate (use Warnf for failures like the
"Failed to compute source hash" message). Apply the same replacement for all
occurrences noted in the review (around the compute-hash block and the ranges
mentioned: 80-95, 101-110, 128-129, 136-137, 309-311) so all build output is
routed through the scenario logger instead of stdout.
- Around line 61-64: The cache-file check in the image build path currently
assumes the cache hash (cacheFile/currentHash) proves the local Kind registry
contains the image, which can be false if a fresh registry reused the same host
port; update the logic in acceptance/kubernetes/kind/image.go (around cacheFile,
currentHash and k.registryPort) to verify the image actually exists in the
registry before skipping the build: after reading cacheFile and matching
currentHash, perform an HTTP registry check (e.g., query the registry on
k.registryPort for the image manifest or tag for the cli image with currentHash)
and only return nil when that manifest/tags check succeeds; if the manifest is
missing, fall through to rebuild the image and then update the cacheFile with
currentHash.
---
Duplicate comments:
In `@acceptance/kubernetes/kind/image.go`:
- Around line 279-282: Instead of overwriting steps[i].ComputeResources with an
empty corev1.ResourceRequirements{}, remove only the CPU request entry so memory
requests/limits and other settings remain intact: locate the code that assigns
steps[i].ComputeResources = corev1.ResourceRequirements{} and change it to
delete the corev1.ResourceCPU key from steps[i].ComputeResources.Requests (and
leave Limits and other Request entries untouched), ensuring you check for a nil
Requests map before deleting so you don't panic.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 98d73663-17b0-4ead-8150-e1a43babbe0b
⛔ Files ignored due to path filters (1)
features/__snapshots__/validate_input.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
.github/workflows/checks-codecov.yamlMakefileacceptance/acceptance_test.goacceptance/cli/cli.goacceptance/conftest/conftest.goacceptance/git/git.goacceptance/go.modacceptance/kubernetes/kind/acceptance.Dockerfileacceptance/kubernetes/kind/image.goacceptance/kubernetes/kind/kind.goacceptance/kubernetes/kind/kubernetes.goacceptance/log/log.goacceptance/log/log_test.goacceptance/registry/registry.goacceptance/testenv/testenv.goacceptance/wiremock/wiremock.gofeatures/validate_input.featurehack/ubi-base-image-bump.sh
✅ Files skipped from review due to trivial changes (4)
- acceptance/testenv/testenv.go
- acceptance/kubernetes/kind/kubernetes.go
- acceptance/kubernetes/kind/acceptance.Dockerfile
- acceptance/log/log_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- hack/ubi-base-image-bump.sh
- acceptance/conftest/conftest.go
- acceptance/log/log.go
- acceptance/go.mod
- .github/workflows/checks-codecov.yaml
Contributor
Author
|
Regarding the outside-diff comment on |
st3penta
force-pushed
the
EC-1710-impl-v3
branch
from
ccec8aa to
7e69603
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
acceptance/kubernetes/kind/image.go (2)
59-60:⚠️ Potential issue | 🟠 MajorRoute these subprocess logs through
log.LoggerFor(ctx).These
fmt.Print*calls still bypass the per-scenario log file, so parallel setup output can interleave on stdout and failures here won't show up in the scenario log path printed on test failure.BuildSnapshotArtifact()already uses the scenario logger;buildCliImage()and themake task-bundlepath should do the same.Also applies to: 79-95, 101-110, 127-137, 309-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/image.go` around lines 59 - 60, The code uses fmt.Printf/Println to emit subprocess/setup logs (e.g., inside buildCliImage(), the make task-bundle path, and around the source-hash message) which bypass per-scenario logs; replace these fmt.Print* calls with the scenario logger obtained via log.LoggerFor(ctx) and route subprocess.Stdout/Stderr to that logger (similar to BuildSnapshotArtifact()). Find and update occurrences referenced in the review (lines around the source-hash message, blocks at 79-95, 101-110, 127-137, and 309-311) so all messages are logged via l := log.LoggerFor(ctx) and emitted through l.Infof/l.Warnf or by piping command output into that logger.
61-64:⚠️ Potential issue | 🟠 MajorDon't treat the hash marker as proof the registry already has the image.
A matching cache file only proves a previous build completed. If the Kind registry was recreated or emptied after that, this returns early and later TaskRuns pull a missing
cliimage. Please verifylocalhost:<port>/cli:<tag>exists before skipping the rebuild.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/image.go` around lines 61 - 64, The cache-hit shortcut currently returns early based only on cacheFile and currentHash (variables cacheFile, currentHash and registryPort), which can be stale if the Kind registry was recreated; update the logic so that after detecting a matching cache file you verify the image actually exists in the local registry (e.g., probe localhost:<registryPort>/v2/cli/manifests/<tag> with an appropriate Accept header or perform an HTTP HEAD/GET on the image manifest) and only skip the rebuild when that registry check succeeds; if the registry probe fails or returns 404, fall through to rebuild and update the cache file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acceptance/cli/cli.go`:
- Around line 563-565: The fallback fmt.Errorf is printing the pointer
representation of *godog.DocString; replace the formatted values to use the
expanded stderr text variables instead: use expectedStdErr (the expanded
expected text) and status.stderr (the actual stderr string) or the diff buffer
b.String() as appropriate. Update the error construction in the block around the
diff.Text call (the bytes.Buffer b, diff.Text(...), and the fmt.Errorf) to pass
expectedStdErr and status.stderr (or b.String() if you want the diff output)
rather than the raw expected *godog.DocString pointer.
In `@acceptance/kubernetes/kind/image.go`:
- Around line 68-74: The build directory ".acceptance-build" used in
buildCliImage() is shared across concurrent runs and can be removed or
overwritten; change buildDir to be unique per scenario by generating a temporary
or scenario-scoped directory (e.g., using ioutil.TempDir or appending a unique
identifier like scenario ID or PID to ".acceptance-build"), ensure os.MkdirAll
uses that unique path, and update the defer os.RemoveAll(buildDir) to remove the
correct unique directory so parallel buildCliImage() calls do not conflict.
---
Duplicate comments:
In `@acceptance/kubernetes/kind/image.go`:
- Around line 59-60: The code uses fmt.Printf/Println to emit subprocess/setup
logs (e.g., inside buildCliImage(), the make task-bundle path, and around the
source-hash message) which bypass per-scenario logs; replace these fmt.Print*
calls with the scenario logger obtained via log.LoggerFor(ctx) and route
subprocess.Stdout/Stderr to that logger (similar to BuildSnapshotArtifact()).
Find and update occurrences referenced in the review (lines around the
source-hash message, blocks at 79-95, 101-110, 127-137, and 309-311) so all
messages are logged via l := log.LoggerFor(ctx) and emitted through
l.Infof/l.Warnf or by piping command output into that logger.
- Around line 61-64: The cache-hit shortcut currently returns early based only
on cacheFile and currentHash (variables cacheFile, currentHash and
registryPort), which can be stale if the Kind registry was recreated; update the
logic so that after detecting a matching cache file you verify the image
actually exists in the local registry (e.g., probe
localhost:<registryPort>/v2/cli/manifests/<tag> with an appropriate Accept
header or perform an HTTP HEAD/GET on the image manifest) and only skip the
rebuild when that registry check succeeds; if the registry probe fails or
returns 404, fall through to rebuild and update the cache file.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c4117b4-1c23-4113-a408-b7657a2e513d
⛔ Files ignored due to path filters (1)
features/__snapshots__/validate_input.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
.github/workflows/checks-codecov.yamlMakefileacceptance/acceptance_test.goacceptance/cli/cli.goacceptance/conftest/conftest.goacceptance/go.modacceptance/kubernetes/kind/image.goacceptance/kubernetes/kind/kind.goacceptance/kubernetes/kind/kubernetes.goacceptance/registry/registry.goacceptance/testenv/testenv.gofeatures/validate_input.feature
✅ Files skipped from review due to trivial changes (1)
- acceptance/testenv/testenv.go
🚧 Files skipped from review as they are similar to previous changes (5)
- acceptance/kubernetes/kind/kubernetes.go
- acceptance/go.mod
- acceptance/conftest/conftest.go
- .github/workflows/checks-codecov.yaml
- acceptance/kubernetes/kind/kind.go
st3penta
force-pushed
the
EC-1710-impl-v3
branch
from
7e69603 to
a7aba21
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
acceptance/kubernetes/kind/image.go (1)
155-166:⚠️ Potential issue | 🟠 MajorInclude the version-derivation script in the cache key.
buildCliImage()executeshack/derive-version.shat Line 77 and injects its result into the binary at Line 88, butcomputeSourceHash()never hashes that script. A change there can hit the old cache and skip rebuilding the CLI image.♻️ Suggested fix
buildFiles := []string{ "go.mod", "go.sum", "Dockerfile", "build.sh", "Makefile", + "hack/derive-version.sh", "hack/reduce-snapshot.sh", "tools/kubectl/go.mod", "tools/kubectl/go.sum", "acceptance/kubernetes/kind/acceptance.Dockerfile", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/image.go` around lines 155 - 166, computeSourceHash() currently hashes buildFiles but omits the version-derivation script, so changes to hack/derive-version.sh won't bust the cache used by buildCliImage(); update the buildFiles slice (used by computeSourceHash()) to include "hack/derive-version.sh" (the script run in buildCliImage()) so the cache key reflects changes to the version derivation and triggers a rebuild when that script changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 126-132: The cleanup function currently copies snapshots but
doesn't remove the temporary worktree, leaking directories; update the cleanup()
referenced by trap cleanup EXIT to also remove the ACCEPTANCE_WORKDIR (e.g., run
a safe recursive delete of $${ACCEPTANCE_WORKDIR} after copying snapshots) and
ensure any failure during copy doesn't skip removal so the temp dir is always
deleted on exit.
---
Duplicate comments:
In `@acceptance/kubernetes/kind/image.go`:
- Around line 155-166: computeSourceHash() currently hashes buildFiles but omits
the version-derivation script, so changes to hack/derive-version.sh won't bust
the cache used by buildCliImage(); update the buildFiles slice (used by
computeSourceHash()) to include "hack/derive-version.sh" (the script run in
buildCliImage()) so the cache key reflects changes to the version derivation and
triggers a rebuild when that script changes.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ff3565d-d860-428b-a76a-7c3ddbc5cf7c
⛔ Files ignored due to path filters (1)
features/__snapshots__/validate_input.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
.github/workflows/checks-codecov.yamlMakefileacceptance/acceptance_test.goacceptance/cli/cli.goacceptance/conftest/conftest.goacceptance/go.modacceptance/kubernetes/kind/image.goacceptance/kubernetes/kind/kind.goacceptance/kubernetes/kind/kubernetes.goacceptance/registry/registry.goacceptance/testenv/testenv.gofeatures/validate_input.feature
✅ Files skipped from review due to trivial changes (2)
- acceptance/testenv/testenv.go
- .github/workflows/checks-codecov.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- acceptance/conftest/conftest.go
- features/validate_input.feature
- acceptance/cli/cli.go
- acceptance/go.mod
- acceptance/acceptance_test.go
- acceptance/kubernetes/kind/kubernetes.go
- acceptance/kubernetes/kind/kind.go
st3penta
force-pushed
the
EC-1710-impl-v3
branch
from
a7aba21 to
9a76ac3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
acceptance/conftest/conftest.go (1)
95-115:⚠️ Potential issue | 🟠 MajorKeep the Conftest dump for any failing return, not just
cmd.Run()failures.This now skips the command/stdout/stderr dump when Conftest exits 0 but the later assertions fail (missing
producesfile or content diff). Those failures become much harder to debug in CI. Gate the defer on the function’s final returned error instead of onlycmdErr.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/conftest/conftest.go` around lines 95 - 115, The defer that prints the Conftest command/stdout/stderr currently gates on cmdErr, so it only runs when cmd.Run() fails; change it to gate on the function's final returned error instead. Make the function use a named error return (e.g., "err error" or "retErr error") and update the deferred closure to check that named return (if err == nil { return }) so the dump runs for any non-nil return (including later assertion failures like missing produces or content diffs); leave the existing cmdErr assignment and cmd.Run() call as-is but replace the defer's check from cmdErr to the named return error variable.
♻️ Duplicate comments (1)
Makefile (1)
127-129:⚠️ Potential issue | 🟠 MajorDon't let
cleanup()skiprm -rfon copy errors.Line 128 can fail when no snapshots were produced, and with
-ethat exits the trap function before Line 129 runs. The temp acceptance workdir can still leak on those paths.♻️ Suggested fix
cleanup() { \ - cp "$${ACCEPTANCE_WORKDIR}"/features/__snapshots__/* "$(ROOT_DIR)"/features/__snapshots__/; \ - rm -rf "$${ACCEPTANCE_WORKDIR}"; \ + status=0; \ + if compgen -G "$${ACCEPTANCE_WORKDIR}/features/__snapshots__/*" > /dev/null; then \ + cp "$${ACCEPTANCE_WORKDIR}"/features/__snapshots__/* "$(ROOT_DIR)"/features/__snapshots__/ || status=$$?; \ + fi; \ + rm -rf "$${ACCEPTANCE_WORKDIR}" || status=$$?; \ + return $$status; \ }; \As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 127 - 129, The cleanup() trap can abort early if the cp of "$${ACCEPTANCE_WORKDIR}"/features/__snapshots__/* fails (causing the subsequent rm -rf "$${ACCEPTANCE_WORKDIR}" not to run); modify cleanup() so copy failures do not stop the function—either ignore cp errors (e.g., make the cp non-fatal) or guard the copy with a conditional that checks the source exists/has files—so that rm -rf "$${ACCEPTANCE_WORKDIR}" always executes to remove the temp workdir.
🧹 Nitpick comments (1)
acceptance/kubernetes/kind/image.go (1)
303-314: Consider capturingversionandtasksbeforeg.Go()for clarity and defensive coding.The code is functionally correct with Go 1.25.8 (which the project requires), as Go 1.22+ fixed the loop variable capture issue in closures. The suggested shadowing (e.g.,
version, tasks := version, tasks) is optional defensive hardening to make the intent explicit and protect against future Go version downgrades.Suggested fix
g, gCtx := errgroup.WithContext(ctx) for version, tasks := range taskBundles { + version, tasks := version, tasks g.Go(func() error { tasksPath := strings.Join(tasks, ",") cmd := exec.CommandContext(gCtx, "make", "task-bundle", fmt.Sprintf("TASK_REPO=localhost:%d/ec-task-bundle", k.registryPort), fmt.Sprintf("TASKS=%s", tasksPath), fmt.Sprintf("TASK_TAG=%s", version)) /* `#nosec` */ if out, err := cmd.CombinedOutput(); err != nil { fmt.Printf("[ERROR] Unable to build and push the Task bundle image, %q returned an error: %v\nCommand output:\n", cmd, err) fmt.Print(string(out)) return err } return nil }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/image.go` around lines 303 - 314, The goroutine closure inside the errgroup loop captures the loop variables version and tasks; to make this explicit and defensive, shadow them immediately before launching the goroutine (e.g., localVersion, localTasks := version, tasks) and use those local names inside the g.Go closure and when building the exec.CommandContext call (which invokes make task-bundle and uses TASK_TAG and TASKS). This preserves current behavior while avoiding accidental loop-variable capture if code or Go versions change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acceptance/acceptance_test.go`:
- Around line 153-164: Capture the error returned from testenv.Persist (e.g.
persistErr := err) and treat the scenario as failed if either scenarioErr or
persistErr is non-nil: set a failureErr := scenarioErr; if failureErr == nil {
failureErr = persistErr } then if failureErr != nil call
tracker.addFailure(scenario.Name, scenario.Uri, logFile, failureErr) and do not
os.Remove(logFile); otherwise remove the log for passing runs. Keep returning
the Persist error (return ctx, persistErr) so the hook still fails when
persistence fails.
In `@acceptance/cli/cli.go`:
- Around line 727-730: This mutates the package-global picocolors.SUPPORT_COLOR
(c.SUPPORT_COLOR) from a parallel scenario hook causing a data race; remove the
mutation in the failure handler (the block that calls testenv.NoColorOutput(ctx)
and assigns c.SUPPORT_COLOR) and instead determine coloring policy once during
suite startup (e.g., in your acceptance suite setup or init) and set
picocolors.SUPPORT_COLOR there using a sync.Once or equivalent; if code in this
failure path needs to know whether colors are allowed, read
testenv.NoColorOutput(ctx) into a local variable and use that, but do not write
to c.SUPPORT_COLOR from scenario-level/parallel code.
In `@acceptance/kubernetes/kind/image.go`:
- Around line 156-166: computeSourceHash()'s buildFiles list (the buildFiles
slice in acceptance/kubernetes/kind/image.go) omits hack/derive-version.sh so
changes to that script don't update the cache key; add "hack/derive-version.sh"
to the buildFiles slice used by computeSourceHash() (the same list that
currently includes "build.sh", "Dockerfile", etc.) so the script is included in
the source hash and the CLI image rebuilds when the derive-version script
changes.
---
Outside diff comments:
In `@acceptance/conftest/conftest.go`:
- Around line 95-115: The defer that prints the Conftest command/stdout/stderr
currently gates on cmdErr, so it only runs when cmd.Run() fails; change it to
gate on the function's final returned error instead. Make the function use a
named error return (e.g., "err error" or "retErr error") and update the deferred
closure to check that named return (if err == nil { return }) so the dump runs
for any non-nil return (including later assertion failures like missing produces
or content diffs); leave the existing cmdErr assignment and cmd.Run() call as-is
but replace the defer's check from cmdErr to the named return error variable.
---
Duplicate comments:
In `@Makefile`:
- Around line 127-129: The cleanup() trap can abort early if the cp of
"$${ACCEPTANCE_WORKDIR}"/features/__snapshots__/* fails (causing the subsequent
rm -rf "$${ACCEPTANCE_WORKDIR}" not to run); modify cleanup() so copy failures
do not stop the function—either ignore cp errors (e.g., make the cp non-fatal)
or guard the copy with a conditional that checks the source exists/has files—so
that rm -rf "$${ACCEPTANCE_WORKDIR}" always executes to remove the temp workdir.
---
Nitpick comments:
In `@acceptance/kubernetes/kind/image.go`:
- Around line 303-314: The goroutine closure inside the errgroup loop captures
the loop variables version and tasks; to make this explicit and defensive,
shadow them immediately before launching the goroutine (e.g., localVersion,
localTasks := version, tasks) and use those local names inside the g.Go closure
and when building the exec.CommandContext call (which invokes make task-bundle
and uses TASK_TAG and TASKS). This preserves current behavior while avoiding
accidental loop-variable capture if code or Go versions change.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ae2153b-86e8-488d-9645-822a2184f752
⛔ Files ignored due to path filters (1)
features/__snapshots__/validate_input.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
Makefileacceptance/acceptance_test.goacceptance/cli/cli.goacceptance/conftest/conftest.goacceptance/go.modacceptance/kubernetes/kind/image.goacceptance/kubernetes/kind/kind.goacceptance/testenv/testenv.gofeatures/validate_input.feature
✅ Files skipped from review due to trivial changes (2)
- features/validate_input.feature
- acceptance/testenv/testenv.go
🚧 Files skipped from review as they are similar to previous changes (1)
- acceptance/go.mod
Comment on lines
+727
to
730
| noColors := testenv.NoColorOutput(ctx) | ||
| if c.SUPPORT_COLOR != !noColors { | ||
| c.SUPPORT_COLOR = !noColors | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Writes to picocolors global color switch:"
rg -n --type go 'c\.SUPPORT_COLOR\s*=' acceptance
echo
echo "Parallel scenario execution configuration:"
sed -n '210,216p' acceptance/acceptance_test.goRepository: conforma/cli
Length of output: 465
Avoid mutating picocolors.SUPPORT_COLOR from parallel scenario hooks.
c.SUPPORT_COLOR is package-global state. This code runs during failure handling while scenarios execute in parallel (lines 212–213 of acceptance_test.go), creating a data race. Concurrent scenario failures will corrupt the color setting across tests. Configure color policy once at suite setup instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@acceptance/cli/cli.go` around lines 727 - 730, This mutates the
package-global picocolors.SUPPORT_COLOR (c.SUPPORT_COLOR) from a parallel
scenario hook causing a data race; remove the mutation in the failure handler
(the block that calls testenv.NoColorOutput(ctx) and assigns c.SUPPORT_COLOR)
and instead determine coloring policy once during suite startup (e.g., in your
acceptance suite setup or init) and set picocolors.SUPPORT_COLOR there using a
sync.Once or equivalent; if code in this failure path needs to know whether
colors are allowed, read testenv.NoColorOutput(ctx) into a local variable and
use that, but do not write to c.SUPPORT_COLOR from scenario-level/parallel code.
Contributor
Author
There was a problem hiding this comment.
This mutation predates this PR. The diff shows we only moved it within the same function, not introduced it. Fixing it properly (setting color policy once at suite init) would be a separate concern. Leaving as-is for this PR.
There was a problem hiding this comment.
@st3penta, understood — the move is cosmetic and the race was pre-existing. Would you like me to open a follow-up issue to track fixing the c.SUPPORT_COLOR global mutation (set it once at suite init) so it isn't lost?
😔 ((\_/) (•ᴗ•) / >🐛)
The make push-image step was one of the slowest steps in CI even when source had not changed. Compute a SHA-256 hash of all build inputs (Go source, go.mod, go.sum, Dockerfile, build.sh, Makefile, hack/reduce-snapshot.sh) and compare it against a per-registry-port cache marker file. When the hash matches, skip the build entirely. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ref: https://issues.redhat.com/browse/EC-1710
The sequential per-version task bundle loop was the slowest step in CI, much slower than the same step locally. Each version produces an independent bundle image, so the builds are safe to run concurrently. Use errgroup to propagate the first error and cancel remaining builds via context. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ref: https://issues.redhat.com/browse/EC-1710
After applying cluster resources, wait only for the in-cluster registry before starting image builds. The CLI image build and task bundle build now run concurrently with each other and with the Tekton Pipelines deployment, since they only need the registry to push to. Significantly reduces CI setup time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ref: https://issues.redhat.com/browse/EC-1710
The acceptance job's Go build cache was broken: it used actions/cache/restore (read-only) with path '**' and key 'main', which did not match the real cache paths (~/.cache/go-build, ~/go/pkg/mod). Switch to read-write actions/cache targeting the correct directories with a content-based key from go.sum. Download pre-built tkn and kubectl binaries (versions extracted from tools/go.mod and tools/kubectl/go.mod) instead of compiling from source every run. The Makefile and image.go prefer binaries from PATH when available, falling back to go build locally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ref: https://issues.redhat.com/browse/EC-1710
st3penta
force-pushed
the
EC-1710-impl-v3
branch
from
9a76ac3 to
1ce2bb4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Makefile (1)
127-129:⚠️ Potential issue | 🟠 MajorMake
cleanup()delete the temp worktree even when snapshot copy fails.With
-eenabled, a failingcpon Line 128 exitscleanup()before Line 129 runs, so the temp checkout can still leak on the same failure path this target was trying to fix. Make the delete unconditional and preserve the copy status separately.♻️ Suggested fix
cleanup() { \ - cp "$${ACCEPTANCE_WORKDIR}"/features/__snapshots__/* "$(ROOT_DIR)"/features/__snapshots__/; \ - rm -rf "$${ACCEPTANCE_WORKDIR}"; \ + cp_status=0; \ + cp "$${ACCEPTANCE_WORKDIR}"/features/__snapshots__/* "$(ROOT_DIR)"/features/__snapshots__/ || cp_status=$$?; \ + rm -rf "$${ACCEPTANCE_WORKDIR}"; \ + [ $$cp_status -eq 0 ]; \ }; \As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 127 - 129, The cleanup() target currently runs cp and then rm -rf but with -e enabled a failing cp aborts before the rm; modify cleanup() so the snapshot copy (the cp "$${ACCEPTANCE_WORKDIR}"/features/__snapshots__/* "$(ROOT_DIR)"/features/__snapshots__/) is executed and its exit status is saved to a variable, then unconditionally run rm -rf "$${ACCEPTANCE_WORKDIR}" to always remove the temp worktree, and finally return/exit with the saved cp status so the overall result still reflects whether the copy failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acceptance/kubernetes/kind/image.go`:
- Around line 155-185: The cache key currently skips the entire "acceptance"
tree in the filepath.WalkDir callback, which allows changes to
acceptance/kubernetes/kind/image.go to be ignored; update the hashing logic to
include the Kind image build code by either adding
"acceptance/kubernetes/kind/image.go" to the explicit buildFiles slice
(alongside go.mod, Dockerfile, etc.) or by changing the SkipDir condition to
only skip "acceptance" subtrees except the "acceptance/kubernetes/kind" path so
that the image.go file is hashed (refer to the buildFiles slice and the
anonymous WalkDir function that currently returns filepath.SkipDir for
d.Name()=="acceptance"). Ensure the chosen change results in hashing
acceptance/kubernetes/kind/image.go so cache invalidation occurs when it
changes.
---
Duplicate comments:
In `@Makefile`:
- Around line 127-129: The cleanup() target currently runs cp and then rm -rf
but with -e enabled a failing cp aborts before the rm; modify cleanup() so the
snapshot copy (the cp "$${ACCEPTANCE_WORKDIR}"/features/__snapshots__/*
"$(ROOT_DIR)"/features/__snapshots__/) is executed and its exit status is saved
to a variable, then unconditionally run rm -rf "$${ACCEPTANCE_WORKDIR}" to
always remove the temp worktree, and finally return/exit with the saved cp
status so the overall result still reflects whether the copy failed.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cff6135b-9cb4-4b0f-9445-b303680f6afd
⛔ Files ignored due to path filters (1)
features/__snapshots__/validate_input.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
.github/workflows/checks-codecov.yamlMakefileacceptance/acceptance_test.goacceptance/cli/cli.goacceptance/conftest/conftest.goacceptance/go.modacceptance/kubernetes/kind/image.goacceptance/kubernetes/kind/kind.goacceptance/kubernetes/kind/kubernetes.goacceptance/registry/registry.goacceptance/testenv/testenv.gofeatures/validate_input.feature
✅ Files skipped from review due to trivial changes (1)
- acceptance/testenv/testenv.go
🚧 Files skipped from review as they are similar to previous changes (6)
- acceptance/conftest/conftest.go
- features/validate_input.feature
- acceptance/go.mod
- acceptance/kubernetes/kind/kubernetes.go
- .github/workflows/checks-codecov.yaml
- acceptance/kubernetes/kind/kind.go
Print the test suite start time so the user can estimate when it will finish, and output total elapsed time at the end. The shell runs with -e, so a go test failure would abort before the timing echo. Capture the exit code and re-propagate it after printing duration and collecting coverage so timing info survives test failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ref: https://issues.redhat.com/browse/EC-1710
st3penta
force-pushed
the
EC-1710-impl-v3
branch
from
1ce2bb4 to
7bff9b0
Compare
Contributor
Author
|
Addressed the Makefile cleanup leak (cp is now non-fatal so For the remaining items from the latest CodeRabbit reviews:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
acceptance/kubernetes/kind/image.go (1)
354-367:⚠️ Potential issue | 🟠 MajorPropagate the context returned by
log.LoggerForonce.
log.LoggerForonly memoizes the per-scenario file logger on the returned context (acceptance/log/log.go:156-179). Ignoring that value here means a plain context will create a fresh temp log file on Line 354 and again on Line 366, splitting output and leaking files.💡 Suggested fix
+ logger, ctx := log.LoggerFor(ctx) for _, name := range fileNames { fileDescriptor, err := fs.Add(ctx, name, mediaType, "") if err != nil { return ctx, fmt.Errorf("failed to add file %s: %w", name, err) } fileDescriptors = append(fileDescriptors, fileDescriptor) t := testenv.FetchState[testState](ctx) if t != nil { t.snapshotDigest = fileDescriptor.Digest.String() } - logger, _ := log.LoggerFor(ctx) logger.Logf("file descriptor for %s: %v", name, fileDescriptor) } @@ - logger, _ := log.LoggerFor(ctx) logger.Log("manifest descriptor:", manifestDescriptor)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/image.go` around lines 354 - 367, The code calls log.LoggerFor(ctx) twice and discards the returned context, causing new temp log files to be created; call log.LoggerFor once, capture both the returned logger and the updated context (e.g., ctxWithLog := ctxFromLogger) and use that propagated context for subsequent logging and operations so the memoized per-scenario file logger is reused (update the occurrences around the fileDescriptor log and the manifest descriptor log to use the same returned context/logger pair from log.LoggerFor).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acceptance/acceptance_test.go`:
- Around line 148-153: The scenario logger is being closed before calling
testenv.Persist(ctx), which causes teardown/persist logs to be lost; move the
call to logger.Close() to after the Persist call so that testenv.Persist(ctx)
can write into the open per-scenario log file — update the sequence around
logger, ctx := log.LoggerFor(ctx), logger.LogFile(), testenv.Persist(ctx) and
logger.Close() so Close() runs only after Persist completes.
---
Outside diff comments:
In `@acceptance/kubernetes/kind/image.go`:
- Around line 354-367: The code calls log.LoggerFor(ctx) twice and discards the
returned context, causing new temp log files to be created; call log.LoggerFor
once, capture both the returned logger and the updated context (e.g., ctxWithLog
:= ctxFromLogger) and use that propagated context for subsequent logging and
operations so the memoized per-scenario file logger is reused (update the
occurrences around the fileDescriptor log and the manifest descriptor log to use
the same returned context/logger pair from log.LoggerFor).
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b6219fd6-0f68-434f-b52d-588341e36993
⛔ Files ignored due to path filters (1)
features/__snapshots__/validate_input.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
Makefileacceptance/acceptance_test.goacceptance/cli/cli.goacceptance/conftest/conftest.goacceptance/go.modacceptance/kubernetes/kind/image.goacceptance/kubernetes/kind/kind.goacceptance/testenv/testenv.gofeatures/validate_input.feature
✅ Files skipped from review due to trivial changes (1)
- features/validate_input.feature
🚧 Files skipped from review as they are similar to previous changes (4)
- acceptance/testenv/testenv.go
- acceptance/cli/cli.go
- acceptance/kubernetes/kind/kind.go
- acceptance/go.mod
st3penta
force-pushed
the
EC-1710-impl-v3
branch
from
7bff9b0 to
d0eadf6
Compare
Contributor
Author
|
Regarding the outside-diff comment on |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
acceptance/kubernetes/kind/image.go (1)
156-166:⚠️ Potential issue | 🟠 MajorInclude
acceptance/kubernetes/kind/image.goin the CLI cache hash.This file now defines artifact-affecting build behavior (
ldflags, version injection, local binary build flow), butcomputeSourceHashstill skips the wholeacceptancetree at Line 184. A change here can therefore hit the old marker and silently reuse a stale CLI image.♻️ Proposed fix
buildFiles := []string{ "go.mod", "go.sum", "Dockerfile", "build.sh", "Makefile", "hack/derive-version.sh", "hack/reduce-snapshot.sh", "tools/kubectl/go.mod", "tools/kubectl/go.sum", "acceptance/kubernetes/kind/acceptance.Dockerfile", + "acceptance/kubernetes/kind/image.go", }Also applies to: 177-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/image.go` around lines 156 - 166, computeSourceHash currently excludes the entire acceptance tree so changes in acceptance/kubernetes/kind/image.go (which sets ldflags, version injection and local build flow) are ignored and can reuse stale CLI images; update computeSourceHash to stop skipping the whole acceptance directory and instead include this file (or at minimum acceptance/kubernetes/kind/image.go) in the hash calculation, or add a targeted exception that explicitly hashes the contents of the buildFiles/ldflags logic defined in image.go so changes to functions/variables around ldflags, version injection, or local binary build flow will alter the computed source hash.acceptance/kubernetes/kind/kind.go (1)
348-350:⚠️ Potential issue | 🟠 MajorRefresh discovery before resolving custom-resource mappings.
Line 348 resolves every GVK from
k.mapper, but that mapper was built before cluster setup installed any CRDs. IfapplyResourcesis reused on later manifests with Tekton or Application API objects, this can fail on a stale RESTMapper even though the CRD already exists in-cluster.🔍 Verification script
#!/bin/bash set -euo pipefail printf 'Call sites for applyResources:\n' rg -n 'applyResources\s*\(' acceptance -C2 printf '\nRESTMapper construction / refresh points:\n' rg -n 'GetAPIGroupResources|NewDiscoveryRESTMapper' acceptance/kubernetes/kind -C2 printf '\nCustom apiVersions present in YAML inputs:\n' rg -n '^\s*apiVersion:\s*(tekton\.dev|appstudio\.redhat\.com|results\.tekton\.dev|chains\.tekton\.dev|operator\.tekton\.dev|route\.openshift\.io|monitoring\.coreos\.com|triggers\.tekton\.dev)' \ -g '*.yaml' -g '*.yml' acceptance features tasks hack -C1 || trueExpected result: if
applyResourceshas call sites beyond initial cluster bootstrap and those manifests include custom API groups, the mapper needs to be rebuilt after CRD installation or resolved lazily at apply time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/kind.go` around lines 348 - 350, applyResources is resolving each object's GVK against the pre-built RESTMapper (k.mapper.RESTMapping) that was created before CRDs were installed, causing stale mapping failures when applyResources is reused later; update applyResources to refresh or lazily rebuild the RESTMapper before calling k.mapper.RESTMapping (e.g., call the same discovery logic used when constructing the mapper via GetAPIGroupResources/NewDiscoveryRESTMapper or recreate k.mapper) so that CRDs installed earlier in the bootstrap are discovered, then resolve the mapping and proceed as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@acceptance/kubernetes/kind/image.go`:
- Around line 156-166: computeSourceHash currently excludes the entire
acceptance tree so changes in acceptance/kubernetes/kind/image.go (which sets
ldflags, version injection and local build flow) are ignored and can reuse stale
CLI images; update computeSourceHash to stop skipping the whole acceptance
directory and instead include this file (or at minimum
acceptance/kubernetes/kind/image.go) in the hash calculation, or add a targeted
exception that explicitly hashes the contents of the buildFiles/ldflags logic
defined in image.go so changes to functions/variables around ldflags, version
injection, or local binary build flow will alter the computed source hash.
In `@acceptance/kubernetes/kind/kind.go`:
- Around line 348-350: applyResources is resolving each object's GVK against the
pre-built RESTMapper (k.mapper.RESTMapping) that was created before CRDs were
installed, causing stale mapping failures when applyResources is reused later;
update applyResources to refresh or lazily rebuild the RESTMapper before calling
k.mapper.RESTMapping (e.g., call the same discovery logic used when constructing
the mapper via GetAPIGroupResources/NewDiscoveryRESTMapper or recreate k.mapper)
so that CRDs installed earlier in the bootstrap are discovered, then resolve the
mapping and proceed as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bbfd51fc-1638-4c1b-b6d0-9116a6ca1128
⛔ Files ignored due to path filters (1)
features/__snapshots__/validate_input.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
acceptance/acceptance_test.goacceptance/cli/cli.goacceptance/conftest/conftest.goacceptance/go.modacceptance/kubernetes/kind/image.goacceptance/kubernetes/kind/kind.goacceptance/testenv/testenv.gofeatures/validate_input.feature
✅ Files skipped from review due to trivial changes (1)
- acceptance/testenv/testenv.go
🚧 Files skipped from review as they are similar to previous changes (3)
- acceptance/conftest/conftest.go
- features/validate_input.feature
- acceptance/cli/cli.go
simonbaird
previously approved these changes
Apr 22, 2026
Member
simonbaird
left a comment
simonbaird
left a comment
There was a problem hiding this comment.
Fantastic! Appreciate the clean and well described commits.
Member
|
Ran in 5m36s for me locally. 👍 Not sure if it's intentionally removed, but I missed these nice output lines, e.g.: Can we bring them back? |
Switch godog formatter from "pretty" to "progress" (overridable via -format flag or EC_ACCEPTANCE_FORMAT env var), with output suppressed by default via progress:/dev/null. Gate logExecution and conftest debug output on scenario or command failure so passing scenarios stay silent. Route snapshot artifact debug prints through the per-scenario file logger instead of stdout. Suppress k8s client-side throttling warnings by disabling klog logtostderr. Move failed scenario summaries and the profiling report to TestMain so they appear after all go test output. Gate verbose execution details behind a -verbose flag and use diff for stderr assertions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ref: https://issues.redhat.com/browse/EC-1710
Remove CPU resource requests from task bundle steps in the acceptance tests to eliminate the Tekton scheduling waterfall. Each TaskRun pod requested 2600m CPU, limiting concurrent pods to 1-2 on CI (3.5 allocatable CPUs), which effectively serialized 26 Kind scenarios. Without the requests all pods schedule immediately. Set imagePullPolicy: IfNotPresent on CLI image steps as a defensive measure. Parallelize namespaced resource application in applyResources by applying cluster-scoped resources first, then namespaced resources concurrently via errgroup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ref: https://issues.redhat.com/browse/EC-1710
The ensureConfigMapRBAC() call lived only in CreateConfigMap, so scenarios that never create a ConfigMap (like "Collect keyless signing parameters when the namespace does not exist") relied on another scenario running first to set up the ClusterRole and ClusterRoleBinding. Parallel execution broke this assumption. Move the call into Start() right after applyResources, so RBAC is in place before any scenario runs. The call in CreateConfigMap stays as an idempotent guard. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ref: https://issues.redhat.com/browse/EC-1710
Two scenarios in validate_input.feature both write policy.yaml
and input.json to the shared repo root. When godog schedules
them concurrently, one overwrites the other's files before ec
reads them, causing spurious exit code mismatches.
Write per-scenario files to ${TMPDIR} instead, which is unique
per scenario and avoids the collision.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ref: https://issues.redhat.com/browse/EC-1710
st3penta
force-pushed
the
EC-1710-impl-v3
branch
from
d0eadf6 to
47fe005
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
acceptance/kubernetes/kind/image.go (1)
354-388:⚠️ Potential issue | 🟠 MajorReuse the
LoggerForresult once and return its context.Lines 354 and 366 call
log.LoggerFor(ctx)but discard the returned context. Whenctxlacks an embedded logger, each call creates a separate temp log file. The loop at line 354 repeats this per iteration, fragmenting log output across multiple files instead of consolidating into one scenario log.Call
LoggerForonce at the function start, capture both the logger and context, and propagate the returned context through the function.Proposed fix
+ logger, ctx := log.LoggerFor(ctx) for _, name := range fileNames { fileDescriptor, err := fs.Add(ctx, name, mediaType, "") if err != nil { return ctx, fmt.Errorf("failed to add file %s: %w", name, err) } fileDescriptors = append(fileDescriptors, fileDescriptor) t := testenv.FetchState[testState](ctx) if t != nil { t.snapshotDigest = fileDescriptor.Digest.String() } - logger, _ := log.LoggerFor(ctx) logger.Logf("file descriptor for %s: %v", name, fileDescriptor) } - logger, _ := log.LoggerFor(ctx) logger.Log("manifest descriptor:", manifestDescriptor)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/image.go` around lines 354 - 388, The function repeatedly calls log.LoggerFor(ctx) and discards the returned context which creates multiple temp log files; instead call log.LoggerFor once at the start of the function, capture both returned logger and the new context (e.g., logger, ctx = log.LoggerFor(ctx)), then use that ctx for all subsequent operations (including the loop that logs file descriptors, the manifest creation and tagging, and the oras.Copy call) so all log output uses the same logger/context; replace other log.LoggerFor(ctx) calls with uses of the captured logger and ensure functions like oras.PackManifest, fs.Tag, remote.NewRepository and oras.Copy receive the updated ctx.
♻️ Duplicate comments (1)
acceptance/cli/cli.go (1)
727-730:⚠️ Potential issue | 🔴 CriticalParallel failure hooks still mutate global color state.
c.SUPPORT_COLORis package-global and this write happens in scenario-level code running concurrently, so failures can race and flip shared output state unpredictably. This should be set once at suite init and treated as read-only in scenario hooks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/cli/cli.go` around lines 727 - 730, The code mutates the package-global c.SUPPORT_COLOR in scenario-level hooks (using testenv.NoColorOutput(ctx)), causing races; change this to set c.SUPPORT_COLOR once during suite initialization (e.g., TestMain or an init/suite-setup function) and make scenario code read it only. Remove the write to c.SUPPORT_COLOR from the scenario-level block and instead compute noColors := testenv.NoColorOutput(ctx) into a local variable if needed, or read the already-initialized c.SUPPORT_COLOR; ensure any assignment of c.SUPPORT_COLOR happens only in the single suite init location.
🧹 Nitpick comments (1)
acceptance/acceptance_test.go (1)
163-170: Serialize/dev/ttywrites to avoid interleaved status lines in parallel runs.These writes happen from parallel scenario hooks and can still produce mixed output; a small lock around the write keeps pass/fail lines stable.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Proposed diff
type scenarioTracker struct { mu sync.Mutex failedScenarios []failedScenario } +var ttyOutputMu sync.Mutex + @@ if tty, err := os.OpenFile("/dev/tty", os.O_WRONLY, 0); err == nil { + ttyOutputMu.Lock() if scenarioErr != nil || persistErr != nil { fmt.Fprintf(tty, "✗ FAILED: %s (%s)\n", scenario.Name, scenario.Uri) } else { fmt.Fprintf(tty, "✓ PASSED: %s (%s)\n", scenario.Name, scenario.Uri) } + ttyOutputMu.Unlock() tty.Close() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/acceptance_test.go` around lines 163 - 170, Parallel writes to /dev/tty cause interleaved status lines; add a shared mutex (e.g., ttyWriteMu sync.Mutex) and wrap the writes in the block that opens /dev/tty—specifically put ttyWriteMu.Lock() before the fmt.Fprintf calls and defer/ensure ttyWriteMu.Unlock() after the writes (and before/after tty.Close()) so the fmt.Fprintf calls that reference scenario.Name, scenario.Uri and the condition using scenarioErr/persistErr are serialized; add the sync import and a package-level mutex variable to locate and apply the change around the os.OpenFile("/dev/tty", ...) block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@acceptance/kubernetes/kind/image.go`:
- Around line 354-388: The function repeatedly calls log.LoggerFor(ctx) and
discards the returned context which creates multiple temp log files; instead
call log.LoggerFor once at the start of the function, capture both returned
logger and the new context (e.g., logger, ctx = log.LoggerFor(ctx)), then use
that ctx for all subsequent operations (including the loop that logs file
descriptors, the manifest creation and tagging, and the oras.Copy call) so all
log output uses the same logger/context; replace other log.LoggerFor(ctx) calls
with uses of the captured logger and ensure functions like oras.PackManifest,
fs.Tag, remote.NewRepository and oras.Copy receive the updated ctx.
---
Duplicate comments:
In `@acceptance/cli/cli.go`:
- Around line 727-730: The code mutates the package-global c.SUPPORT_COLOR in
scenario-level hooks (using testenv.NoColorOutput(ctx)), causing races; change
this to set c.SUPPORT_COLOR once during suite initialization (e.g., TestMain or
an init/suite-setup function) and make scenario code read it only. Remove the
write to c.SUPPORT_COLOR from the scenario-level block and instead compute
noColors := testenv.NoColorOutput(ctx) into a local variable if needed, or read
the already-initialized c.SUPPORT_COLOR; ensure any assignment of
c.SUPPORT_COLOR happens only in the single suite init location.
---
Nitpick comments:
In `@acceptance/acceptance_test.go`:
- Around line 163-170: Parallel writes to /dev/tty cause interleaved status
lines; add a shared mutex (e.g., ttyWriteMu sync.Mutex) and wrap the writes in
the block that opens /dev/tty—specifically put ttyWriteMu.Lock() before the
fmt.Fprintf calls and defer/ensure ttyWriteMu.Unlock() after the writes (and
before/after tty.Close()) so the fmt.Fprintf calls that reference scenario.Name,
scenario.Uri and the condition using scenarioErr/persistErr are serialized; add
the sync import and a package-level mutex variable to locate and apply the
change around the os.OpenFile("/dev/tty", ...) block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fac0f6a-48c4-46cb-a948-69812410f800
⛔ Files ignored due to path filters (1)
features/__snapshots__/validate_input.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
acceptance/acceptance_test.goacceptance/cli/cli.goacceptance/conftest/conftest.goacceptance/go.modacceptance/kubernetes/kind/image.goacceptance/kubernetes/kind/kind.goacceptance/testenv/testenv.gofeatures/validate_input.feature
✅ Files skipped from review due to trivial changes (1)
- acceptance/testenv/testenv.go
🚧 Files skipped from review as they are similar to previous changes (4)
- acceptance/conftest/conftest.go
- acceptance/go.mod
- acceptance/kubernetes/kind/kind.go
- features/validate_input.feature
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The acceptance tests were slow in CI and hard to debug when
they failed. Most of the wall-clock time went into sequential
image builds and a setup phase that blocked on each step
finishing before the next could start. On top of that, test
output was noisy and interleaved across parallel goroutines,
making failure diagnosis a chore.
The setup phase now overlaps image builds with the Tekton
deployment, builds task bundles in parallel, and skips the
CLI image build entirely when source hasn't changed. The old
multi-stage Docker build has been replaced with a host-side
binary injection into a minimal base image, and CI downloads
pre-built tool binaries instead of compiling them from source.
Kind scenario scheduling no longer bottlenecks on CPU resource
requests that serialized pod creation.
Logging has been reworked so each scenario writes to its own
file, eliminating cross-goroutine interleaving. Passing runs
are now quiet by default, and failed scenario summaries with
log file paths are printed at the end. Test containers are
properly terminated after each scenario instead of
accumulating for the entire run.
The increased parallelism exposed a couple of latent race
conditions: two validate_input scenarios were writing to the
same files in the repo root, and ConfigMap RBAC setup depended
on scenario execution order. Both have been fixed so tests
run reliably regardless of scheduling.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Ref: https://issues.redhat.com/browse/EC-1710