forgejo_admin/basketball-api

Fork 0

fix: add retry loop to CI wget for update-kustomize-tag #216

Merged

forgejo_admin merged 2 commits from 230-ci-wget-retry into main

2026-03-28 20:07:45 +00:00

forgejo_admin commented

2026-03-28 19:54:42 +00:00

Owner

Summary

BusyBox wget has no built-in retry flags. Transient forgejo-http connection failures cause the kustomize tag update step to fail silently. This wraps the wget call in a 5-attempt retry loop with 5s backoff and a fatal exit if all attempts fail.

Changes

.woodpecker.yaml: Replaced bare wget call in update-kustomize-tag step with a retry loop (5 attempts, 5s sleep between retries, fatal on total failure)

Test Plan

YAML validates: python3 -c "import yaml; yaml.safe_load(open('.woodpecker.yaml'))"
Merge to main triggers Woodpecker pipeline; verify update-kustomize-tag step succeeds
Simulate transient failure: temporarily break the URL, confirm retry log lines appear

Forgejo issue: forgejo_admin/pal-e-platform#230

🤖 Generated with Claude Code

## Summary BusyBox wget has no built-in retry flags. Transient forgejo-http connection failures cause the kustomize tag update step to fail silently. This wraps the wget call in a 5-attempt retry loop with 5s backoff and a fatal exit if all attempts fail. ## Changes - `.woodpecker.yaml`: Replaced bare `wget` call in `update-kustomize-tag` step with a retry loop (5 attempts, 5s sleep between retries, fatal on total failure) ## Test Plan - [ ] YAML validates: `python3 -c "import yaml; yaml.safe_load(open('.woodpecker.yaml'))"` - [ ] Merge to main triggers Woodpecker pipeline; verify `update-kustomize-tag` step succeeds - [ ] Simulate transient failure: temporarily break the URL, confirm retry log lines appear ## Related - Forgejo issue: forgejo_admin/pal-e-platform#230 🤖 Generated with [Claude Code](https://claude.com/claude-code)

forgejo_admin added 1 commit

2026-03-28 19:54:42 +00:00

fix: add retry loop to CI wget for update-kustomize-tag

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

523ba391fa

BusyBox wget has no retry flags. Transient forgejo-http connection
failures cause the kustomize tag update to fail silently. 5 retries
with 5s backoff.

Ref: forgejo_admin/pal-e-platform#230

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-28 20:04:41 +00:00

Author

Owner

PR #216 Review

DOMAIN REVIEW

Tech stack: Woodpecker CI YAML (shell scripting, YAML block scalars, Woodpecker variable substitution)

YAML block scalar analysis -- This is the core issue. The PR changes the update-kustomize-tag step from a bare wget to a retry loop but keeps the >- (folded block scalar) from the original. This worked before because all lines (wget, --header, URL) were at the same base indentation and >- folded them into a single line. The new code introduces a for loop, pushing the wget and its arguments to a deeper indentation level (10 spaces vs 8 spaces base). Under YAML >- rules, lines more-indented than the first content line have their newlines preserved, not folded. This means the shell receives:

for i in 1 2 3 4 5; do
  wget -O /tmp/update-kustomize-tag.sh
  --header="Authorization: token ${FORGEJO_TOKEN}"
  "http://forgejo-http.forgejo.svc.cluster.local:80/..."
  && break;
  echo "wget attempt $i failed, retrying in 5s...";
  sleep 5;
done; test -f /tmp/update-kustomize-tag.sh || { echo "FATAL: ..."; exit 1; }

The wget command and its --header/URL arguments are on separate shell lines with no backslash continuations. The shell executes wget -O /tmp/update-kustomize-tag.sh alone (which likely errors or downloads nothing), then tries to run --header="..." as a standalone command (fails). The && break never fires because it's chained to the URL line, not wget's exit code.

Cross-repo comparison: Both westside-app/.woodpecker.yaml (line 58) and pal-e-app/.woodpecker.yaml (line 89) implement the identical retry pattern but use | (literal block scalar) with \ backslash continuations on the wget arguments. That is the correct approach.

$$ escaping: The change from ${FORGEJO_TOKEN} to $${FORGEJO_TOKEN} is correct. Woodpecker interprets single-$ expressions as its own substitution; $$ passes a literal $ to the shell, which then reads the environment variable injected via from_secret. The original single-$ was a latent bug (worked because Woodpecker passes through unknown variable names). Same applies to $$i in the echo statement.

BLOCKERS

1. >- folded scalar breaks multi-line wget command -- As detailed above, the >- block scalar preserves newlines for the more-indented lines inside the for loop body, causing the wget command to be split across multiple shell lines without continuations. This will cause the step to fail on every run.

Fix: Change >- to | and add \ backslash continuations on the wget --header and URL lines. Reference the working pattern in /home/ldraney/westside-app/.woodpecker.yaml (lines 58-68) or /home/ldraney/pal-e-app/.woodpecker.yaml (lines 89-97).

NITS

1. test -f vs test -s -- The guard uses test -f (file exists) but test -s (file exists AND non-empty) is safer. A zero-byte download (e.g., HTTP 200 with empty body on a transient error) would pass test -f but fail test -s. The pal-e-app version already uses test -s. The westside-app version also uses test -f (same gap). Consider adopting test -s here as the stronger check.

2. DRY observation -- This retry-loop-plus-wget pattern is now copy-pasted across three repos (basketball-api, westside-app, pal-e-app) with minor divergences (>- vs |, test -f vs test -s, auth header vs no auth). Not a blocker for this PR, but the pattern is ripe for extraction into a shared wrapper script or Woodpecker plugin.

SOP COMPLIANCE

Branch named after issue: 230-ci-wget-retry matches pal-e-platform#230
PR body has Summary, Changes, Test Plan, Related sections
Related section references the parent issue (forgejo_admin/pal-e-platform#230)
Related does not reference a plan slug (N/A -- this is a standalone bugfix, no plan expected)
No secrets committed (token comes from from_secret)
No scope creep -- single file, single concern
Commit message is descriptive
YAML parse validation noted in Test Plan

PROCESS OBSERVATIONS

Change failure risk: HIGH in current form. The >- scalar issue means this PR would break the kustomize tag update step on every pipeline run, preventing ArgoCD deployments. The fix is straightforward (change to | + add \ continuations).
Cross-repo divergence: Three repos now have slightly different implementations of the same retry pattern. A future issue to extract this into a shared approach would reduce maintenance burden and prevent exactly this kind of scalar-style bug.
YAML validation gap: The Test Plan correctly includes yaml.safe_load() validation, but YAML parse validation only checks syntax -- it cannot catch the semantic issue where >- produces a different string than intended. Shell-level validation (e.g., shellcheck or manual inspection of the folded output) would be needed.

VERDICT: NOT APPROVED

The >- folded block scalar will break the wget command by preserving newlines inside the for-loop body. Change to | with backslash continuations on the wget arguments, matching the proven pattern in westside-app and pal-e-app. The test -f to test -s upgrade is a nit.

## PR #216 Review ### DOMAIN REVIEW **Tech stack**: Woodpecker CI YAML (shell scripting, YAML block scalars, Woodpecker variable substitution) **YAML block scalar analysis** -- This is the core issue. The PR changes the `update-kustomize-tag` step from a bare `wget` to a retry loop but keeps the `>-` (folded block scalar) from the original. This worked before because all lines (`wget`, `--header`, URL) were at the same base indentation and `>-` folded them into a single line. The new code introduces a `for` loop, pushing the `wget` and its arguments to a deeper indentation level (10 spaces vs 8 spaces base). Under YAML `>-` rules, lines more-indented than the first content line have their **newlines preserved**, not folded. This means the shell receives: ``` for i in 1 2 3 4 5; do wget -O /tmp/update-kustomize-tag.sh --header="Authorization: token ${FORGEJO_TOKEN}" "http://forgejo-http.forgejo.svc.cluster.local:80/..." && break; echo "wget attempt $i failed, retrying in 5s..."; sleep 5; done; test -f /tmp/update-kustomize-tag.sh || { echo "FATAL: ..."; exit 1; } ``` The wget command and its `--header`/URL arguments are on **separate shell lines with no backslash continuations**. The shell executes `wget -O /tmp/update-kustomize-tag.sh` alone (which likely errors or downloads nothing), then tries to run `--header="..."` as a standalone command (fails). The `&& break` never fires because it's chained to the URL line, not wget's exit code. **Cross-repo comparison**: Both `westside-app/.woodpecker.yaml` (line 58) and `pal-e-app/.woodpecker.yaml` (line 89) implement the identical retry pattern but use `|` (literal block scalar) with `\` backslash continuations on the wget arguments. That is the correct approach. **`$$` escaping**: The change from `${FORGEJO_TOKEN}` to `$${FORGEJO_TOKEN}` is correct. Woodpecker interprets single-`$` expressions as its own substitution; `$$` passes a literal `$` to the shell, which then reads the environment variable injected via `from_secret`. The original single-`$` was a latent bug (worked because Woodpecker passes through unknown variable names). Same applies to `$$i` in the echo statement. ### BLOCKERS **1. `>-` folded scalar breaks multi-line wget command** -- As detailed above, the `>-` block scalar preserves newlines for the more-indented lines inside the `for` loop body, causing the wget command to be split across multiple shell lines without continuations. This will cause the step to fail on every run. **Fix**: Change `>-` to `|` and add `\` backslash continuations on the wget `--header` and URL lines. Reference the working pattern in `/home/ldraney/westside-app/.woodpecker.yaml` (lines 58-68) or `/home/ldraney/pal-e-app/.woodpecker.yaml` (lines 89-97). ### NITS **1. `test -f` vs `test -s`** -- The guard uses `test -f` (file exists) but `test -s` (file exists AND non-empty) is safer. A zero-byte download (e.g., HTTP 200 with empty body on a transient error) would pass `test -f` but fail `test -s`. The `pal-e-app` version already uses `test -s`. The `westside-app` version also uses `test -f` (same gap). Consider adopting `test -s` here as the stronger check. **2. DRY observation** -- This retry-loop-plus-wget pattern is now copy-pasted across three repos (`basketball-api`, `westside-app`, `pal-e-app`) with minor divergences (`>-` vs `|`, `test -f` vs `test -s`, auth header vs no auth). Not a blocker for this PR, but the pattern is ripe for extraction into a shared wrapper script or Woodpecker plugin. ### SOP COMPLIANCE - [x] Branch named after issue: `230-ci-wget-retry` matches `pal-e-platform#230` - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related section references the parent issue (`forgejo_admin/pal-e-platform#230`) - [ ] Related does not reference a plan slug (N/A -- this is a standalone bugfix, no plan expected) - [x] No secrets committed (token comes from `from_secret`) - [x] No scope creep -- single file, single concern - [x] Commit message is descriptive - [x] YAML parse validation noted in Test Plan ### PROCESS OBSERVATIONS - **Change failure risk**: HIGH in current form. The `>-` scalar issue means this PR would break the kustomize tag update step on every pipeline run, preventing ArgoCD deployments. The fix is straightforward (change to `|` + add `\` continuations). - **Cross-repo divergence**: Three repos now have slightly different implementations of the same retry pattern. A future issue to extract this into a shared approach would reduce maintenance burden and prevent exactly this kind of scalar-style bug. - **YAML validation gap**: The Test Plan correctly includes `yaml.safe_load()` validation, but YAML parse validation only checks syntax -- it cannot catch the semantic issue where `>-` produces a different string than intended. Shell-level validation (e.g., `shellcheck` or manual inspection of the folded output) would be needed. ### VERDICT: NOT APPROVED The `>-` folded block scalar will break the wget command by preserving newlines inside the for-loop body. Change to `|` with backslash continuations on the wget arguments, matching the proven pattern in `westside-app` and `pal-e-app`. The `test -f` to `test -s` upgrade is a nit.

forgejo_admin added 1 commit

2026-03-28 20:05:44 +00:00

fix: use literal block scalar and test -s for wget retry

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

0e6c3828cd

QA caught that >- (folded) breaks the multi-line for-loop into invalid
shell. Changed to | (literal). Also test -s catches zero-byte artifacts.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-28 20:07:27 +00:00

Author

Owner

PR #216 Review (Re-review after fix commit)

DOMAIN REVIEW

Tech stack: Woodpecker CI YAML (shell scripting in BusyBox alpine/git container).

Previous rejection raised two issues:

BLOCKER: >- (folded block scalar) collapsed the for-loop into a single line, breaking the shell syntax.
NIT: test -f only checks file existence, not content; test -s is needed to catch zero-byte downloads.

Fix verification -- both resolved:

Block scalar is now | (literal), preserving newlines. The for-loop, wget with backslash continuations, and test guard all render correctly as multi-line shell. FIXED.
Guard is now test -s (exists AND non-empty), catching both missing-file and empty-file failure modes. FIXED.

Additional code review:

$$i and $${FORGEJO_TOKEN} correctly use Woodpecker double-dollar escaping to prevent variable interpolation at YAML parse time.
Retry logic (5 attempts, 5s backoff, && break on success) is sound. All exit paths covered: success breaks loop, total failure hits test -s guard with exit 1.
from_secret: forgejo_token -- no hardcoded credentials.
The | block and sh /tmp/update-kustomize-tag.sh are separate YAML list entries under commands:, so step failure on the download block correctly prevents script execution.
Only .woodpecker.yaml changed (1 file, +10/-4 lines). No scope creep.

BLOCKERS

None. Previous blockers resolved.

NITS

None.

SOP COMPLIANCE

Branch named after issue (230-ci-wget-retry references pal-e-platform#230)
PR body follows template (Summary, Changes, Test Plan, Related)
Related references parent issue (forgejo_admin/pal-e-platform#230)
No secrets committed
No scope creep (single file, single concern)
YAML parse validation in test plan

PROCESS OBSERVATIONS

Cross-repo fix pattern: issue lives on pal-e-platform (#230), code change on basketball-api. This is the first consumer repo getting the retry wrapper around the shared update-kustomize-tag.sh download step. Issue #206 (pal-e-platform) tracks rollout to all 8 app repos -- once this PR validates, the pattern can be replicated. Good MTTR improvement for transient network failures in CI.

VERDICT: APPROVED

## PR #216 Review (Re-review after fix commit) ### DOMAIN REVIEW **Tech stack**: Woodpecker CI YAML (shell scripting in BusyBox alpine/git container). Previous rejection raised two issues: 1. **BLOCKER**: `>-` (folded block scalar) collapsed the for-loop into a single line, breaking the shell syntax. 2. **NIT**: `test -f` only checks file existence, not content; `test -s` is needed to catch zero-byte downloads. **Fix verification -- both resolved:** - Block scalar is now `|` (literal), preserving newlines. The for-loop, wget with backslash continuations, and test guard all render correctly as multi-line shell. **FIXED.** - Guard is now `test -s` (exists AND non-empty), catching both missing-file and empty-file failure modes. **FIXED.** **Additional code review:** - `$$i` and `$${FORGEJO_TOKEN}` correctly use Woodpecker double-dollar escaping to prevent variable interpolation at YAML parse time. - Retry logic (5 attempts, 5s backoff, `&& break` on success) is sound. All exit paths covered: success breaks loop, total failure hits `test -s` guard with `exit 1`. - `from_secret: forgejo_token` -- no hardcoded credentials. - The `|` block and `sh /tmp/update-kustomize-tag.sh` are separate YAML list entries under `commands:`, so step failure on the download block correctly prevents script execution. - Only `.woodpecker.yaml` changed (1 file, +10/-4 lines). No scope creep. ### BLOCKERS None. Previous blockers resolved. ### NITS None. ### SOP COMPLIANCE - [x] Branch named after issue (`230-ci-wget-retry` references pal-e-platform#230) - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [x] Related references parent issue (forgejo_admin/pal-e-platform#230) - [x] No secrets committed - [x] No scope creep (single file, single concern) - [x] YAML parse validation in test plan ### PROCESS OBSERVATIONS Cross-repo fix pattern: issue lives on pal-e-platform (#230), code change on basketball-api. This is the first consumer repo getting the retry wrapper around the shared `update-kustomize-tag.sh` download step. Issue #206 (pal-e-platform) tracks rollout to all 8 app repos -- once this PR validates, the pattern can be replicated. Good MTTR improvement for transient network failures in CI. ### VERDICT: APPROVED