KONFLUX-14934: e2e tests are failing on vector-kubearchive untar

[olegbet]

Summary

• Replaced the two jq … | head pipelines with jq-native slicing:
  • message: jq -r '(.status.conditions[0].message // "No message")[0:200]'
  • conditions: jq -r '[.status.conditions[]?][0:3][] | …'
  • Under #!/bin/bash -e + set -o pipefail, head closing the pipe sent jq a SIGPIPE (exit 141) that aborted the whole bootstrap — exactly what killed both Prow runs.
  • jq-native truncation removes the external pipe entirely.
• The soft-refresh branch now matches a set of transient conditions instead of only context deadline exceeded:
  transient_errors='context deadline exceeded|ComparisonError|failed to untar|error reading from server|rpc error'
  So an Unknown app carrying a ComparisonError/failed to untar render flake now gets soft-refreshed and the loop keeps waiting to MAX_SYNC_TIMEOUT, rather than being
  surfaced as a hard failure.

Closes: KONFLUX-14934(https://redhat.atlassian.net/browse/KONFLUX-14934)

Risk assessment

• Low. No disruption is expected.

Validation

Verified: message truncates to 200 chars, conditions cap at 3 lines, and empty input returns empty (fallback
path intact).

Assisted-by: Claude

[github-actions]

Kustomize Render Diff

Comparing eef61f555 → 21e0ea867

No render differences detected.

[qodo-for-redhat-appstudio]

PR Summary by Qodo

Make hack/preview.sh resilient to ArgoCD transient errors and jq|head SIGPIPE

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Avoid jq SIGPIPE failures by truncating/limiting output inside jq under pipefail.
• Treat common ArgoCD reconcile flakes as transient and soft-refresh instead of failing.
• Improve diagnostics stability during bootstrap/wait loops for preview environments.

Diagram

graph TD
  A["hack/preview.sh"] --> B["jq filters"] --> C["App status text"]
  A --> D["oc CLI"] --> E["ArgoCD Applications API"] --> F["App conditions JSON"]
  F --> G["Transient error match"] --> H["Soft refresh annotation"]

  subgraph Legend
    direction LR
    _script["Script"] ~~~ _tool["CLI/Tool"] ~~~ _api{{"API"}} ~~~ _data[("JSON/Data")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep head, handle SIGPIPE explicitly

• ➕ Minimal change to existing jq pipelines
• ➕ Keeps jq expressions simpler for some readers
• ➖ Requires careful shell exception handling (e.g., ignoring exit 141) and can be brittle across pipelines
• ➖ Easy to regress when adding new pipes under set -e/pipefail

2. Narrow transient error matching per known failure modes

• ➕ Reduces risk of masking real failures by over-matching errors
• ➕ Encourages documenting each transient class with rationale and links
• ➖ More maintenance as new transient strings appear across ArgoCD/repo-server/helm
• ➖ May not fix new flakes quickly without frequent updates

Recommendation: The PR’s approach is sound: perform truncation/limiting within jq to avoid pipefail-triggered SIGPIPE aborts, and treat a curated set of known ArgoCD reconcile flakes as transient by soft-refreshing and continuing to wait. Consider periodically reviewing/adjusting the transient error regex to ensure it doesn’t become overly broad and mask persistent failures.

Files changed (1) +14 / -4

Bug fix (1) +14 / -4

preview.shHarden preview bootstrap against pipefail SIGPIPE and ArgoCD flakes +14/-4

Harden preview bootstrap against pipefail SIGPIPE and ArgoCD flakes

• Replaces jq|head pipelines with jq-native slicing to prevent SIGPIPE (exit 141) from aborting the script under set -e and pipefail. Expands the ArgoCD wait loop to classify several common reconcile/render failures (timeouts, comparison errors, untar collisions, rpc/read errors) as transient, triggering a soft refresh and continuing to wait.

hack/preview.sh

[qodo-for-redhat-appstudio]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Empty state treated success ✓ Resolved 🐞 Bug ☼ Reliability

Description

deploy_and_wait_for_argocd() can now incorrectly conclude "all apps are synced" when oc get apps
fails and state becomes empty, because grep -c ... || true yields 0 counts and not_done
becomes empty, triggering the success/break path. This can let the script proceed with ArgoCD not
actually ready, causing downstream steps to run against an unsynced environment.

Code

hack/preview.sh[R637-638]

+        total_apps=$(echo "$state" | grep -c "." || true)
+        synced_apps=$(echo "$state" | grep -c "Synced[[:blank:]]*Healthy" || true)

Relevance

●●● Strong

Clear reliability bug: failed oc output becomes an empty state and is incorrectly interpreted as
successful synchronization.

PR-#13302

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The loop explicitly sets state to "" on oc get failure. With the new grep -c ... || true
logic, empty state yields total_apps=0 and synced_apps=0, and not_done becomes empty; the
next if [ -z "$not_done" ] branch treats that as success and breaks, even though the empty state
can be caused by an oc get error.

hack/preview.sh[633-650]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`state` is set to an empty string when `oc get apps ...` fails. With the new `grep -c ... || true` changes, that empty `state` turns into `total_apps=0`, `synced_apps=0`, and `not_done=""`, which immediately triggers the `if [ -z "$not_done" ]` success branch and breaks the loop.

## Issue Context
This makes transient `oc get` failures look like a successful sync of 0 apps.

## Fix Focus Areas
- hack/preview.sh[633-650]

## Suggested fix
Capture the `oc get` exit status (or explicitly check for empty `state`) and `continue` the loop (after logging + sleeping) instead of allowing the success/break path. For example:

```bash
if ! state=$(oc get apps -n "$ARGOCD_NAMESPACE" --no-headers 2>/dev/null); then
 log_warn "oc get apps failed; retrying"
 sleep "$SYNC_INTERVAL"
 continue
fi

# (optional) also guard the legitimately-empty case
if [ -z "$state" ]; then
 log_warn "oc get apps returned empty; retrying"
 sleep "$SYNC_INTERVAL"
 continue
fi
```

This keeps the `grep -c` fix (no duplicate lines) while preventing false success.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Hidden oc failure cause ✓ Resolved 🐞 Bug ◔ Observability

Description

deploy_and_wait_for_argocd() retries when oc get apps fails, but redirects stderr to /dev/null
and logs only a generic message, so permanent failures (RBAC/auth/wrong namespace) will loop until
timeout without actionable diagnostics.

Code

hack/preview.sh[R636-639]

+        if ! state=$(oc get apps -n $ARGOCD_NAMESPACE --no-headers 2>/dev/null); then
+            log_warn "oc get apps failed; retrying in $SYNC_INTERVAL seconds"
+            sleep $SYNC_INTERVAL
+            continue

Relevance

●●● Strong

Accepted bug precedents favor actionable diagnostics and hardening retry paths; no close rejection
precedent for suppressing stderr.

PR-#13490

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new code explicitly retries on oc get apps failure/empty output, but discards stderr, leaving
only generic warnings and making root-cause debugging difficult during repeated failures.

hack/preview.sh[633-644]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`oc get apps` failures are retried, but the underlying error text is suppressed (`2>/dev/null`). This turns permanent failures into long waits with no actionable debugging context.

### Issue Context
The retry logic was added to avoid treating an empty `$state` as success, which is good. However, without stderr output, operators cannot distinguish transient API flake from hard failures (RBAC/auth/namespace).

### Fix Focus Areas
- hack/preview.sh[633-645]

### Suggested fix
- Capture stderr to a temporary variable/file and include it in the warning log when `oc get apps` fails.
 - Example pattern:
   - `err=$(mktemp)`
   - `if ! state=$(oc get ... --no-headers 2>"$err"); then log_warn "oc get apps failed: $(<"$err")"; ...; fi`
   - Ensure the temp file is removed.
- (Optional) Consider counting consecutive failures and failing fast after N tries instead of waiting the full `MAX_SYNC_TIMEOUT` for clearly permanent errors.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Refresh counters leak globally ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The per-app refresh counters are written via printf -v "$refresh_var" ... without declaring the
target variable local, so transient_refresh_* variables leak into the global shell scope. This can
carry stale counters into later invocations (or sourced usage) and make Unknown-state handling
depend on prior shell state.

Code

hack/preview.sh[R681-685]

+                local refresh_var="transient_refresh_${app//[^a-zA-Z0-9]/_}"
+                local refresh_count=${!refresh_var:-0}
+                if echo "$error" | grep -qE "$transient_errors" && [ "$refresh_count" -lt "$MAX_TRANSIENT_REFRESHES" ]; then
+                    refresh_count=$((refresh_count + 1))
+                    printf -v "$refresh_var" '%s' "$refresh_count"

Relevance

●●● Strong

Clear shell-scoping bug; dynamic printf target remains global despite local metadata variables.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The script is executed with -e and pipefail, and the new code writes counters into dynamically
named variables without local scoping, which makes them persist in the shell environment beyond the
loop/function.

hack/preview.sh[1-2]
hack/preview.sh[676-686]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`printf -v "$refresh_var" ...` writes to a dynamically named variable that is never declared `local`, so it becomes (or remains) a global shell variable. This leaks state (`transient_refresh_*`) outside `deploy_and_wait_for_argocd()` and can affect later calls in the same shell.

### Issue Context
`hack/preview.sh` runs with `#!/bin/bash -e` and `set -o pipefail`, so unexpected state and side effects make failures harder to diagnose and can change control flow across calls.

### Fix Focus Areas
- hack/preview.sh[676-686]

### Suggested change
Declare the dynamically named counter variable as local before reading/writing it, e.g.:

```bash
local refresh_var="transient_refresh_${app//[^a-zA-Z0-9]/_}"
local "$refresh_var"   # ensures the counter is scoped to this function
local refresh_count=${!refresh_var:-0}
...
printf -v "$refresh_var" '%s' "$refresh_count"
```

(Optionally) unset/initialize these locals at the start of the function if you want to guarantee no environment-provided value can seed them.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Bash4-only associative array ✓ Resolved 🐞 Bug ☼ Reliability

Description

deploy_and_wait_for_argocd() now uses local -A transient_refresh_count, which is unsupported on
Bash 3.x and will error when executed; with the script’s #!/bin/bash -e, that error will typically
terminate the whole preview flow. This is a compatibility regression for environments where
/bin/bash is older (notably Bash 3.x).

Code

hack/preview.sh[R605-608]

+    # Bound the soft-refresh recovery per app so a permanent failure that merely
+    # matches a transient message can't loop until MAX_SYNC_TIMEOUT.
+    local -A transient_refresh_count
+

Relevance

●●● Strong

Concrete Bash 3 compatibility regression; reliability findings and runtime failures are typically
accepted, with no close rejection precedent.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The script is run with -e (errexit), so a failing local -A declaration can abort execution when
it’s reached. The PR adds the associative array declaration to implement per-app refresh budgeting.

hack/preview.sh[1-2]
hack/preview.sh[605-608]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`hack/preview.sh` introduces a Bash-4-only feature (`local -A` associative arrays). On Bash 3.x this statement fails, and because the script is executed with `-e`, the preview script will typically exit when it reaches this code path.

### Issue Context
The soft-refresh budget is tracked via an associative array `transient_refresh_count`.

### Fix Focus Areas
- hack/preview.sh[1-2]
- hack/preview.sh[605-608]

### Suggested fix
Choose one:
1) **Add an explicit Bash >= 4 guard** near the top of the script (right after `set -o pipefail`), e.g.:
  - check `BASH_VERSINFO[0]` and `exit 1` with a clear error message including `$BASH_VERSION`.
2) **Avoid associative arrays** by using a Bash-3-compatible structure (e.g., parallel arrays for app names/counts, or a simple key/value string map with helper functions).

Either approach prevents unexpected hard failures on older `/bin/bash`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View medium (2)

5. Refresh budget burns on failure ✓ Resolved 🐞 Bug ☼ Reliability

Description

The refresh counter is incremented before attempting the oc patch, but the patch failure is
explicitly ignored (|| true), so a failed refresh still consumes the per-app budget. This can
prematurely exhaust MAX_TRANSIENT_REFRESHES and escalate to the non-transient diagnostics path
even though no soft refresh was successfully applied.

Code

hack/preview.sh[R683-686]

+                if echo "$error" | grep -qE "$transient_errors" && [ "$refresh_count" -lt "$MAX_TRANSIENT_REFRESHES" ]; then
+                    refresh_count=$((refresh_count + 1))
+                    printf -v "$refresh_var" '%s' "$refresh_count"
+                    log_warn "Application '$app' hit a transient error, attempting soft refresh ($refresh_count/$MAX_TRANSIENT_REFRESHES)"

Relevance

●● Moderate

Plausible retry-budget issue, but history does not establish whether budgets count attempts or
successful refreshes.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new logic increments/persists refresh_count before the refresh patch, while the patch is made
non-fatal, so failures still advance the counter toward the cap.

hack/preview.sh[683-688]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The code increments and persists `refresh_count` before issuing the soft-refresh patch, but `oc patch ... || true` ignores failures. This means unsuccessful refresh attempts still burn the limited refresh budget.

### Issue Context
The intent of `MAX_TRANSIENT_REFRESHES` is to bound recovery attempts per app. Consuming that budget when the refresh request was not applied can cause early escalation and misleading logs.

### Fix Focus Areas
- hack/preview.sh[683-688]

### Suggested change
Move the counter increment after a successful patch, e.g.:

```bash
if echo "$error" | grep -qE "$transient_errors" && [ "$refresh_count" -lt "$MAX_TRANSIENT_REFRESHES" ]; then
 if oc patch applications.argoproj.io "$app" -n "$ARGOCD_NAMESPACE" --type merge \
     -p='{"metadata": {"annotations":{"argocd.argoproj.io/refresh": "soft"}}}' 2>/dev/null; then
   refresh_count=$((refresh_count + 1))
   printf -v "$refresh_var" '%s' "$refresh_count"
   log_warn "Application '$app' hit a transient error, attempting soft refresh ($refresh_count/$MAX_TRANSIENT_REFRESHES)"
   ...
   continue 2
 else
   log_warn "Soft refresh patch for '$app' failed; not counting toward refresh budget"
 fi
fi
```

This preserves the intended cap while avoiding budget depletion on no-op/failed refresh requests.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Overbroad transient error match ✓ Resolved 🐞 Bug ☼ Reliability

Description

deploy_and_wait_for_argocd() now treats any condition containing strings like rpc error or
error reading from server as transient, triggering soft refresh and skipping the immediate
error-reporting path; this can misclassify permanent failures (e.g., auth/RBAC/repo issues) and
defer actionable diagnostics until the global sync timeout. This increases the chance that genuinely
broken deployments only fail after long waits instead of failing fast with clear error output.

Code

hack/preview.sh[R667-669]

+                transient_errors='context deadline exceeded|ComparisonError|failed to untar|error reading from server|rpc error'
+                if echo "$error" | grep -qE "$transient_errors"; then
+                    log_warn "Application '$app' hit a transient error, attempting soft refresh"

Relevance

●● Moderate

Risky retry-policy broadening lacks closely matching acceptance or rejection precedent.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new regex includes broad alternations (error reading from server|rpc error) and, on match, the
code patches a soft refresh and then uses continue 2, skipping the detailed error reporting for
that app/iteration; the surrounding loop only hard-fails when MAX_SYNC_TIMEOUT is hit.

hack/preview.sh[11-15]
hack/preview.sh[604-624]
hack/preview.sh[656-683]
hack/preview.sh[685-688]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`deploy_and_wait_for_argocd()` expanded the transient-error detection to include very broad substrings (e.g., `rpc error`, `error reading from server`). Because the transient branch soft-refreshes and `continue 2`s, it can repeatedly suppress the detailed error path and delay failure until the global timeout.

### Issue Context
This logic is executed for apps in `Unknown` state during the ArgoCD sync wait loop. A global timeout exists, but the new transient classification can turn immediate, actionable failures into long-running timeouts.

### Fix Focus Areas
- hack/preview.sh[656-688]
- hack/preview.sh[11-15]
- hack/preview.sh[604-624]

### Suggested fix
- Narrow the regex to specific known transient messages (e.g., `context deadline exceeded`, a specific `helm pull --untar` collision string), or match on structured fields (condition `type`/`reason`) instead of broad message substrings.
- Add a per-app retry counter/backoff: after N soft refreshes for the same app, fall through to `show_app_details` (or fail early) so permanent issues don’t wait for the full `MAX_SYNC_TIMEOUT`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Global transient_errors variable ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

transient_errors is assigned without local inside deploy_and_wait_for_argocd(), so it becomes
a global shell variable and can leak into later script logic. This increases fragility and the risk
of accidental reuse/collision during future edits.

Code

hack/preview.sh[667]

+                transient_errors='context deadline exceeded|ComparisonError|failed to untar|error reading from server|rpc error'

Relevance

●●● Strong

Adding local is a trivial deterministic shell-scope fix.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The function declares several locals but not transient_errors; the assignment therefore
creates/overwrites a global variable.

hack/preview.sh[538-543]
hack/preview.sh[663-669]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
A variable named `transient_errors` is assigned in `deploy_and_wait_for_argocd()` without `local`, which makes it global in Bash.

### Issue Context
The function already declares other locals, but `transient_errors` is not among them.

### Fix Focus Areas
- hack/preview.sh[538-543]
- hack/preview.sh[663-669]

### Suggested fix
- Declare it as `local transient_errors='...'` (either near the other `local` declarations at function start, or inline as `local transient_errors=...` before the `grep`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. Misleading Unknown-state error ✓ Resolved 🐞 Bug ◔ Observability

Description

After expanding the transient-match set beyond context deadline exceeded, the later error log
still claims the app is Unknown “without 'context deadline exceeded'”, which is no longer the actual
gate for the diagnostic path. This can mislead operators reading logs when investigating
Unknown-state failures.

Code

hack/preview.sh[R667-669]

+                transient_errors='context deadline exceeded|ComparisonError|failed to untar|error reading from server|rpc error'
+                if echo "$error" | grep -qE "$transient_errors"; then
+                    log_warn "Application '$app' hit a transient error, attempting soft refresh"

Relevance

●●● Strong

Updating the stale diagnostic is a straightforward observability fix.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The transient condition check was broadened, but the subsequent log message still references only
the old single-string condition, making it stale relative to the new behavior.

hack/preview.sh[663-669]
hack/preview.sh[685-688]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The Unknown-state diagnostic log message still references only `context deadline exceeded`, but transient handling now includes multiple strings.

### Issue Context
The message is printed when an Unknown-state app does *not* match the transient set and the script falls through to detailed reporting.

### Fix Focus Areas
- hack/preview.sh[663-688]

### Suggested fix
- Change the log line to something accurate, e.g.:
 - `log_error "Application '$app' is in Unknown state without a recognized transient error"`
 - optionally include the transient regex or the extracted condition message to aid debugging.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

✅ Compliance rules (platform): 3 rules

Tip of the day

💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 0361d5e ⚖️ Balanced

Results up to commit 26fb209 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Overbroad transient error match ✓ Resolved 🐞 Bug ☼ Reliability

Description

deploy_and_wait_for_argocd() now treats any condition containing strings like rpc error or
error reading from server as transient, triggering soft refresh and skipping the immediate
error-reporting path; this can misclassify permanent failures (e.g., auth/RBAC/repo issues) and
defer actionable diagnostics until the global sync timeout. This increases the chance that genuinely
broken deployments only fail after long waits instead of failing fast with clear error output.

Code

hack/preview.sh[R667-669]

+                transient_errors='context deadline exceeded|ComparisonError|failed to untar|error reading from server|rpc error'
+                if echo "$error" | grep -qE "$transient_errors"; then
+                    log_warn "Application '$app' hit a transient error, attempting soft refresh"

Relevance

●● Moderate

Risky retry-policy broadening lacks closely matching acceptance or rejection precedent.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new regex includes broad alternations (error reading from server|rpc error) and, on match, the
code patches a soft refresh and then uses continue 2, skipping the detailed error reporting for
that app/iteration; the surrounding loop only hard-fails when MAX_SYNC_TIMEOUT is hit.

hack/preview.sh[11-15]
hack/preview.sh[604-624]
hack/preview.sh[656-683]
hack/preview.sh[685-688]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`deploy_and_wait_for_argocd()` expanded the transient-error detection to include very broad substrings (e.g., `rpc error`, `error reading from server`). Because the transient branch soft-refreshes and `continue 2`s, it can repeatedly suppress the detailed error path and delay failure until the global timeout.

### Issue Context
This logic is executed for apps in `Unknown` state during the ArgoCD sync wait loop. A global timeout exists, but the new transient classification can turn immediate, actionable failures into long-running timeouts.

### Fix Focus Areas
- hack/preview.sh[656-688]
- hack/preview.sh[11-15]
- hack/preview.sh[604-624]

### Suggested fix
- Narrow the regex to specific known transient messages (e.g., `context deadline exceeded`, a specific `helm pull --untar` collision string), or match on structured fields (condition `type`/`reason`) instead of broad message substrings.
- Add a per-app retry counter/backoff: after N soft refreshes for the same app, fall through to `show_app_details` (or fail early) so permanent issues don’t wait for the full `MAX_SYNC_TIMEOUT`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Misleading Unknown-state error ✓ Resolved 🐞 Bug ◔ Observability

Description

After expanding the transient-match set beyond context deadline exceeded, the later error log
still claims the app is Unknown “without 'context deadline exceeded'”, which is no longer the actual
gate for the diagnostic path. This can mislead operators reading logs when investigating
Unknown-state failures.

Code

hack/preview.sh[R667-669]

+                transient_errors='context deadline exceeded|ComparisonError|failed to untar|error reading from server|rpc error'
+                if echo "$error" | grep -qE "$transient_errors"; then
+                    log_warn "Application '$app' hit a transient error, attempting soft refresh"

Relevance

●●● Strong

Updating the stale diagnostic is a straightforward observability fix.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The transient condition check was broadened, but the subsequent log message still references only
the old single-string condition, making it stale relative to the new behavior.

hack/preview.sh[663-669]
hack/preview.sh[685-688]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The Unknown-state diagnostic log message still references only `context deadline exceeded`, but transient handling now includes multiple strings.

### Issue Context
The message is printed when an Unknown-state app does *not* match the transient set and the script falls through to detailed reporting.

### Fix Focus Areas
- hack/preview.sh[663-688]

### Suggested fix
- Change the log line to something accurate, e.g.:
 - `log_error "Application '$app' is in Unknown state without a recognized transient error"`
 - optionally include the transient regex or the extracted condition message to aid debugging.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Global transient_errors variable ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

transient_errors is assigned without local inside deploy_and_wait_for_argocd(), so it becomes
a global shell variable and can leak into later script logic. This increases fragility and the risk
of accidental reuse/collision during future edits.

Code

hack/preview.sh[667]

+                transient_errors='context deadline exceeded|ComparisonError|failed to untar|error reading from server|rpc error'

Relevance

●●● Strong

Adding local is a trivial deterministic shell-scope fix.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The function declares several locals but not transient_errors; the assignment therefore
creates/overwrites a global variable.

hack/preview.sh[538-543]
hack/preview.sh[663-669]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
A variable named `transient_errors` is assigned in `deploy_and_wait_for_argocd()` without `local`, which makes it global in Bash.

### Issue Context
The function already declares other locals, but `transient_errors` is not among them.

### Fix Focus Areas
- hack/preview.sh[538-543]
- hack/preview.sh[663-669]

### Suggested fix
- Declare it as `local transient_errors='...'` (either near the other `local` declarations at function start, or inline as `local transient_errors=...` before the `grep`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit e873e8e ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Bash4-only associative array ✓ Resolved 🐞 Bug ☼ Reliability

Description

deploy_and_wait_for_argocd() now uses local -A transient_refresh_count, which is unsupported on
Bash 3.x and will error when executed; with the script’s #!/bin/bash -e, that error will typically
terminate the whole preview flow. This is a compatibility regression for environments where
/bin/bash is older (notably Bash 3.x).

Code

hack/preview.sh[R605-608]

+    # Bound the soft-refresh recovery per app so a permanent failure that merely
+    # matches a transient message can't loop until MAX_SYNC_TIMEOUT.
+    local -A transient_refresh_count
+

Relevance

●●● Strong

Concrete Bash 3 compatibility regression; reliability findings and runtime failures are typically
accepted, with no close rejection precedent.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The script is run with -e (errexit), so a failing local -A declaration can abort execution when
it’s reached. The PR adds the associative array declaration to implement per-app refresh budgeting.

hack/preview.sh[1-2]
hack/preview.sh[605-608]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`hack/preview.sh` introduces a Bash-4-only feature (`local -A` associative arrays). On Bash 3.x this statement fails, and because the script is executed with `-e`, the preview script will typically exit when it reaches this code path.

### Issue Context
The soft-refresh budget is tracked via an associative array `transient_refresh_count`.

### Fix Focus Areas
- hack/preview.sh[1-2]
- hack/preview.sh[605-608]

### Suggested fix
Choose one:
1) **Add an explicit Bash >= 4 guard** near the top of the script (right after `set -o pipefail`), e.g.:
  - check `BASH_VERSINFO[0]` and `exit 1` with a clear error message including `$BASH_VERSION`.
2) **Avoid associative arrays** by using a Bash-3-compatible structure (e.g., parallel arrays for app names/counts, or a simple key/value string map with helper functions).

Either approach prevents unexpected hard failures on older `/bin/bash`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 2605318 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Refresh counters leak globally ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The per-app refresh counters are written via printf -v "$refresh_var" ... without declaring the
target variable local, so transient_refresh_* variables leak into the global shell scope. This can
carry stale counters into later invocations (or sourced usage) and make Unknown-state handling
depend on prior shell state.

Code

hack/preview.sh[R681-685]

+                local refresh_var="transient_refresh_${app//[^a-zA-Z0-9]/_}"
+                local refresh_count=${!refresh_var:-0}
+                if echo "$error" | grep -qE "$transient_errors" && [ "$refresh_count" -lt "$MAX_TRANSIENT_REFRESHES" ]; then
+                    refresh_count=$((refresh_count + 1))
+                    printf -v "$refresh_var" '%s' "$refresh_count"

Relevance

●●● Strong

Clear shell-scoping bug; dynamic printf target remains global despite local metadata variables.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The script is executed with -e and pipefail, and the new code writes counters into dynamically
named variables without local scoping, which makes them persist in the shell environment beyond the
loop/function.

hack/preview.sh[1-2]
hack/preview.sh[676-686]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`printf -v "$refresh_var" ...` writes to a dynamically named variable that is never declared `local`, so it becomes (or remains) a global shell variable. This leaks state (`transient_refresh_*`) outside `deploy_and_wait_for_argocd()` and can affect later calls in the same shell.

### Issue Context
`hack/preview.sh` runs with `#!/bin/bash -e` and `set -o pipefail`, so unexpected state and side effects make failures harder to diagnose and can change control flow across calls.

### Fix Focus Areas
- hack/preview.sh[676-686]

### Suggested change
Declare the dynamically named counter variable as local before reading/writing it, e.g.:

```bash
local refresh_var="transient_refresh_${app//[^a-zA-Z0-9]/_}"
local "$refresh_var"   # ensures the counter is scoped to this function
local refresh_count=${!refresh_var:-0}
...
printf -v "$refresh_var" '%s' "$refresh_count"
```

(Optionally) unset/initialize these locals at the start of the function if you want to guarantee no environment-provided value can seed them.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Refresh budget burns on failure ✓ Resolved 🐞 Bug ☼ Reliability

Description

The refresh counter is incremented before attempting the oc patch, but the patch failure is
explicitly ignored (|| true), so a failed refresh still consumes the per-app budget. This can
prematurely exhaust MAX_TRANSIENT_REFRESHES and escalate to the non-transient diagnostics path
even though no soft refresh was successfully applied.

Code

hack/preview.sh[R683-686]

+                if echo "$error" | grep -qE "$transient_errors" && [ "$refresh_count" -lt "$MAX_TRANSIENT_REFRESHES" ]; then
+                    refresh_count=$((refresh_count + 1))
+                    printf -v "$refresh_var" '%s' "$refresh_count"
+                    log_warn "Application '$app' hit a transient error, attempting soft refresh ($refresh_count/$MAX_TRANSIENT_REFRESHES)"

Relevance

●● Moderate

Plausible retry-budget issue, but history does not establish whether budgets count attempts or
successful refreshes.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new logic increments/persists refresh_count before the refresh patch, while the patch is made
non-fatal, so failures still advance the counter toward the cap.

hack/preview.sh[683-688]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The code increments and persists `refresh_count` before issuing the soft-refresh patch, but `oc patch ... || true` ignores failures. This means unsuccessful refresh attempts still burn the limited refresh budget.

### Issue Context
The intent of `MAX_TRANSIENT_REFRESHES` is to bound recovery attempts per app. Consuming that budget when the refresh request was not applied can cause early escalation and misleading logs.

### Fix Focus Areas
- hack/preview.sh[683-688]

### Suggested change
Move the counter increment after a successful patch, e.g.:

```bash
if echo "$error" | grep -qE "$transient_errors" && [ "$refresh_count" -lt "$MAX_TRANSIENT_REFRESHES" ]; then
 if oc patch applications.argoproj.io "$app" -n "$ARGOCD_NAMESPACE" --type merge \
     -p='{"metadata": {"annotations":{"argocd.argoproj.io/refresh": "soft"}}}' 2>/dev/null; then
   refresh_count=$((refresh_count + 1))
   printf -v "$refresh_var" '%s' "$refresh_count"
   log_warn "Application '$app' hit a transient error, attempting soft refresh ($refresh_count/$MAX_TRANSIENT_REFRESHES)"
   ...
   continue 2
 else
   log_warn "Soft refresh patch for '$app' failed; not counting toward refresh budget"
 fi
fi
```

This preserves the intended cap while avoiding budget depletion on no-op/failed refresh requests.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 02f90d3 ⚖️ Balanced

No changes from previous review

Results up to commit f2271af ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Empty state treated success ✓ Resolved 🐞 Bug ☼ Reliability

Description

deploy_and_wait_for_argocd() can now incorrectly conclude "all apps are synced" when oc get apps
fails and state becomes empty, because grep -c ... || true yields 0 counts and not_done
becomes empty, triggering the success/break path. This can let the script proceed with ArgoCD not
actually ready, causing downstream steps to run against an unsynced environment.

Code

hack/preview.sh[R637-638]

+        total_apps=$(echo "$state" | grep -c "." || true)
+        synced_apps=$(echo "$state" | grep -c "Synced[[:blank:]]*Healthy" || true)

Relevance

●●● Strong

Clear reliability bug: failed oc output becomes an empty state and is incorrectly interpreted as
successful synchronization.

PR-#13302

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The loop explicitly sets state to "" on oc get failure. With the new grep -c ... || true
logic, empty state yields total_apps=0 and synced_apps=0, and not_done becomes empty; the
next if [ -z "$not_done" ] branch treats that as success and breaks, even though the empty state
can be caused by an oc get error.

hack/preview.sh[633-650]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`state` is set to an empty string when `oc get apps ...` fails. With the new `grep -c ... || true` changes, that empty `state` turns into `total_apps=0`, `synced_apps=0`, and `not_done=""`, which immediately triggers the `if [ -z "$not_done" ]` success branch and breaks the loop.

## Issue Context
This makes transient `oc get` failures look like a successful sync of 0 apps.

## Fix Focus Areas
- hack/preview.sh[633-650]

## Suggested fix
Capture the `oc get` exit status (or explicitly check for empty `state`) and `continue` the loop (after logging + sleeping) instead of allowing the success/break path. For example:

```bash
if ! state=$(oc get apps -n "$ARGOCD_NAMESPACE" --no-headers 2>/dev/null); then
 log_warn "oc get apps failed; retrying"
 sleep "$SYNC_INTERVAL"
 continue
fi

# (optional) also guard the legitimately-empty case
if [ -z "$state" ]; then
 log_warn "oc get apps returned empty; retrying"
 sleep "$SYNC_INTERVAL"
 continue
fi
```

This keeps the `grep -c` fix (no duplicate lines) while preventing false success.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit c175ffe ⚖️ Balanced

No changes from previous review

Results up to commit 6fe3417 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Hidden oc failure cause ✓ Resolved 🐞 Bug ◔ Observability

Description

deploy_and_wait_for_argocd() retries when oc get apps fails, but redirects stderr to /dev/null
and logs only a generic message, so permanent failures (RBAC/auth/wrong namespace) will loop until
timeout without actionable diagnostics.

Code

hack/preview.sh[R636-639]

+        if ! state=$(oc get apps -n $ARGOCD_NAMESPACE --no-headers 2>/dev/null); then
+            log_warn "oc get apps failed; retrying in $SYNC_INTERVAL seconds"
+            sleep $SYNC_INTERVAL
+            continue

Relevance

●●● Strong

Accepted bug precedents favor actionable diagnostics and hardening retry paths; no close rejection
precedent for suppressing stderr.

PR-#13490

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new code explicitly retries on oc get apps failure/empty output, but discards stderr, leaving
only generic warnings and making root-cause debugging difficult during repeated failures.

hack/preview.sh[633-644]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`oc get apps` failures are retried, but the underlying error text is suppressed (`2>/dev/null`). This turns permanent failures into long waits with no actionable debugging context.

### Issue Context
The retry logic was added to avoid treating an empty `$state` as success, which is good. However, without stderr output, operators cannot distinguish transient API flake from hard failures (RBAC/auth/namespace).

### Fix Focus Areas
- hack/preview.sh[633-645]

### Suggested fix
- Capture stderr to a temporary variable/file and include it in the warning log when `oc get apps` fails.
 - Example pattern:
   - `err=$(mktemp)`
   - `if ! state=$(oc get ... --no-headers 2>"$err"); then log_warn "oc get apps failed: $(<"$err")"; ...; fi`
   - Ensure the temp file is removed.
- (Optional) Consider counting consecutive failures and failing fast after N tries instead of waiting the full `MAX_SYNC_TIMEOUT` for clearly permanent errors.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.38%. Comparing base (6e4b525) to head (0361d5e).
⚠️ Report is 66 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13550   +/-   ##
=======================================
  Coverage   60.38%   60.38%           
=======================================
  Files          24       24           
  Lines        1628     1628           
=======================================
  Hits          983      983           
  Misses        563      563           
  Partials       82       82           

Flag	Coverage Δ
go	60.38% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[qodo-for-redhat-appstudio]

Code review by qodo was updated up to the latest commit e873e8e

[flacatus]

/lgtm
/approve

[qodo-for-redhat-appstudio]

Code review by qodo was updated up to the latest commit 2605318

[konflux-ci-qe-bot]

🤖 Pipeline Failure Analysis

Category: Infrastructure

The pipeline failed because the redhat-appstudio-conformance-tests timed out as a PipelineRun remained in a running state, likely due to underlying environmental misconfigurations or resource issues.

📋 Technical Details

Immediate Cause

The redhat-appstudio-conformance-tests step timed out after 30 minutes because the PipelineRun named konflux-ci-upstream-vzgt-on-push-5ss5m did not complete and remained in a "Running" state. This prevented the conformance tests from concluding successfully.

Contributing Factors

Several environmental and configuration issues contributed to the failure. The gather-extra step reported missing test timing secrets (/tmp/secret/TEST_TIME_INSTALL_START), leading to unbound variables in Prometheus queries and preventing proper metric collection. Additionally, the jq utility encountered an "Exec format error", suggesting a potential binary corruption or incompatibility within the execution environment. A "GitHub API rate limit exceeded" error was also observed during the conformance tests, which could hinder operations dependent on GitHub interactions.

Impact

The stalled PipelineRun directly caused the redhat-appstudio-conformance-tests to exceed its allocated time limit, resulting in the failure of the entire Prow job. The underlying environmental issues further indicate a compromised or unstable testing environment, which impedes reliable pipeline execution.

🔍 Evidence

appstudio-e2e-tests/gather-extra

Category: configuration
Root Cause: The gather-extra step failed because essential test timing secrets were missing from the expected /tmp/secret directory, causing Prometheus queries for job metrics to fail due to unbound variables. Additionally, the jq utility, vital for data processing, encountered an "Exec format error" due to an environmental issue with its binary.

Logs:

artifacts/appstudio-e2e-tests/gather-extra/build-log.txt

cat: /tmp/secret/TEST_TIME_INSTALL_START: No such file or directory

artifacts/appstudio-e2e-tests/gather-extra/build-log.txt

error: Query '${t_install} cluster:capacity:cpu:total:cores         sum(cluster:capacity_cpu_cores:sum)' was not valid: t_install: unbound variable

artifacts/appstudio-e2e-tests/gather-extra/build-log.txt

/bin/bash: line 778: /tmp/jq: cannot execute binary file: Exec format error

artifacts/appstudio-e2e-tests/gather-extra/build-log.txt

[must-gather-zt757] POD 2026-08-17T19:00:56.269364764Z /bin/bash: line 15: /usr/bin/gather_multus_logs: No such file or directory

appstudio-e2e-tests/redhat-appstudio-conformance-tests

Category: test
Root Cause: The conformance test timed out because a PipelineRun, konflux-ci-upstream-vzgt-on-push-5ss5m, remained in a "Running" state and did not complete within the 30-minute test execution limit. This indicates an underlying issue where the build pipeline either hung or took an excessively long time to finish.

Logs:

artifacts/appstudio-e2e-tests/redhat-appstudio-conformance-tests/build-log.txt

E0817 18:18:55.629540   28550 diagnostics.go:321] diagnostic: GitHub API probe (test) GET http://127.0.0.1:41127: status=403 retry-after="" x-ratelimit-limit="60" x-ratelimit-remaining="0" x-ratelimit-reset="" x-ratelimit-resource="core" body-prefix="{\"message\":\"API rate limit exceeded\"}"

artifacts/appstudio-e2e-tests/redhat-appstudio-conformance-tests/build-log.txt

PipelineRun konflux-ci-upstream-vzgt-on-push-5ss5m reason: Running

artifacts/appstudio-e2e-tests/redhat-appstudio-conformance-tests/build-log.txt

panic: test timed out after 30m0s
	running tests:
		TestConformance (30m0s)

artifacts/appstudio-e2e-tests/redhat-appstudio-conformance-tests/build-log.txt

FAIL	github.com/konflux-ci/konflux-ci/test/go-tests/tests/conformance	1800.106s
FAIL

artifacts/appstudio-e2e-tests/redhat-appstudio-conformance-tests/build-log.txt

github.com/konflux-ci/konflux-ci/test/go-tests/pkg/clients/has.(*Controller).WaitForComponentPipelineToBeFinished(0x95523082ff0, 0x955225db808, {0x2b08b9b, 0x5}, {0x95523513530, 0x28}, {0x0, 0x0}, 0x955230889a0, 0x95522de9e60, ...)
	/tmp/tmp.KfdJhi5WAa/test/go-tests/pkg/clients/has/components.go:219 +0x1e5

---

Analysis powered by prow-failure-analysis | Build: 2089410283784114176

[qodo-for-redhat-appstudio]

Code review by qodo was updated up to the latest commit 02f90d3

[qodo-for-redhat-appstudio]

Code review by qodo was updated up to the latest commit f2271af

[qodo-for-redhat-appstudio]

Code review by qodo was updated up to the latest commit c175ffe

[qodo-for-redhat-appstudio]

Code review by qodo was updated up to the latest commit 6fe3417

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: avi-biton, flacatus, olegbet
Once this PR has been reviewed and has the lgtm label, please assign glevi-rh for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

New changes are detected. LGTM label has been removed.

[qodo-for-redhat-appstudio]

Code review by qodo was updated up to the latest commit 0361d5e

[openshift-ci]

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.