feat: reusable kustomize tag update script for CI pipelines #205
No reviewers
Labels
No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo_admin/pal-e-platform!205
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "204-image-tag-automation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
After CI pushes a new image to Harbor, the kustomize overlay
newTaginpal-e-deploymentsis never updated -- every deploy requires a manual PR orkubectl set image. Issue #148 was marked done but the automation was never built (false-done). This PR adds a reusable shell script and Woodpecker step reference that each app repo can adopt to close the gap.Changes
scripts/update-kustomize-tag.sh-- Standalone POSIX shell script that:pal-e-deploymentsvia internal Forgejo URL (no hairpin)newTagentries in the target overlay'skustomization.yamlvia sed[skip ci]prefix to prevent CI loopsscripts/woodpecker-update-tag-step.yaml-- Copy-paste Woodpecker step template with:depends_onandwhenguards (only runs on push to main after build-and-push)Test Plan
latest, multiplenewTagentries)imagesblocks (basketball-api pattern)Review Checklist
Prerequisites for cross-repo rollout (child issues needed):
forgejo_tokensecret?update-deployment-tagtargeting wrong repo)Action items for rollout:
forgejo_tokenas a Woodpecker global secret (or per-repo for repos that lack it)update-kustomize-tagstep to each repo's.woodpecker.yamlusing the reference templateupdate-deployment-tagstep from pal-e-app (it updatesk8s/deployment.yamlin the app repo instead of the kustomize overlay)Related Notes
.woodpecker.yamlrollout +forgejo_tokensecret provisioningQA Review
Findings (self-review, QA agent unavailable)
Fixed in follow-up commit:
Race condition on concurrent pushes (severity: high) -- If two repos push images simultaneously, both clone pal-e-deployments at the same commit. The second
git pushwould fail with a non-fast-forward error. Added retry loop (3 attempts) withgit pull --rebase+ re-apply sed pattern.Shallow clone too shallow for rebase (severity: medium) --
git clone --depth 1cannot performgit pull --rebasebecause there is no common ancestor. Changed to--depth 5so the retry loop has room to rebase.Noted, not fixed (no action needed):
alpine/git:latestimage tag not pinned (severity: low) -- Usinglatestis standard practice for this CI step across the repo. Pinning can be done as a platform-wide hardening pass later.Standalone script vs inline step divergence (severity: info) -- The
.shscript uses\\((shell double-escape) while the YAML inline step uses\((single-escape). Both are correct for their respective contexts -- shell file needs double-escape, YAML string does not.VERDICT: APPROVE (after self-review fixes pushed)
PR #205 Review
DOMAIN REVIEW
Tech stack: POSIX shell scripts + Woodpecker CI YAML (CI/CD automation domain). The PR adds two new files under
scripts/-- a standalone shell script and a Woodpecker step template. Both target kustomize overlays in thepal-e-deploymentsrepo.File paths are clean. Both files are at
scripts/in the repo root, not nested under a worktree. Good.Shell script quality (
scripts/update-kustomize-tag.sh):#!/bin/shwithset -eu-- correct for CI containers (Alpine/busybox)${VAR:?msg}-- good patterngit diff --quiet-- correct[skip ci]commit prefix to prevent CI loops -- correct--depth 5instead of--depth 1to support rebase -- correct, with clear comment explaining whyWoodpecker step template (
scripts/woodpecker-update-tag-step.yaml):westside-contractshas no overlay yet -- gooddepends_on: build-and-pushandwhen: push to mainguards -- correctSed pattern analysis against real kustomization.yaml files:
Pattern:
s/^\( newTag:\s*\).*$/\1${IMAGE_TAG}/Validated against all 13
newTaglines across 9 overlay prod kustomization.yaml files:newTag: 4102552649e4...) -- matches correctlynewTag: a81d881) -- matches correctlylatest(e.g.,newTag: latest) -- matches correctlynewTag: "3ecec95"inplatform-validation) -- matches and replaces with unquoted tag. This is a minor format change but functionally equivalent for kustomize. Acceptable.Multi-image overlay behavior:
Several overlays have 2
newTagentries (basketball-api, pal-e-docs, mcd-tracker, pal-e-mail). The sed replaces ALLnewTaglines to the same value. This is documented as intentional in the PR ("update all newTag entries"). Currently all dual entries share the same tag, so this is correct. However, if a future overlay needs different tags per image block, this script would need modification. The PR body's review checklist adequately calls this out as known scope.BLOCKERS
1. DRY violation: full logic duplication between
.shscript and.yamltemplatescripts/update-kustomize-tag.sh(98 lines) contains the complete implementation.scripts/woodpecker-update-tag-step.yaml(79 lines) re-implements the same logic inline undercommands:rather than calling the shell script. The YAML template comment says "Inline implementation (no external script dependency)" -- but this creates two copies of the same logic that will diverge over time.This is a DRY violation in a CI/security-adjacent path (pushing commits with a token to a deployment repo). If a bug is found in the retry logic, the sed pattern, or the git auth, it must be fixed in two places. The
.shscript exists specifically to be reusable -- the YAML template should call it (e.g.,wgetorcurlthe script from pal-e-platform, or have each consuming repo copy the.shand call it).Recommendation: Either (a) have the YAML template call the
.shscript (fetch from raw Forgejo URL or expect it vendored), or (b) remove the.shscript entirely and document that the YAML inline block IS the canonical implementation. Two copies of push-with-token retry logic is a divergence risk.2. No test coverage for the shell script
The script performs git clone, sed, commit, push with retry logic. There are no automated tests. The Test Plan shows manual verification only ("Verified sed pattern works on all kustomization.yaml variants") but no reproducible test. For a script that pushes commits to a deployment repo with a token, this is a blocker per the review criteria ("New functionality with zero test coverage").
At minimum, the sed pattern should have a test: create a mock
kustomization.yamlwith the known variants (bare SHA, quoted tag,latest, multiplenewTagentries), run the sed, and assert the output. This could be a simple shell script inscripts/tests/or a CI step that runs on PR.NITS
alpine/git:latestimage tag in the YAML template (line 20): Usinglatestfor CI images is fragile. Consider pinning to a specific version (e.g.,alpine/git:2.47) to prevent unexpected breakage when the image updates. This is noted in the k8s security checklist (image tags should be pinned).OVERLAY_ENVdefaulting toprodin the.shscript but hardcoded toprodin the YAML template: The.shscript supportsOVERLAY_ENVas an optional override, but the YAML template hardcodesoverlays/${OVERLAY}/prod/kustomization.yaml. If the.shscript is the intended reusable interface, the YAML template's hardcoding is fine as a prod-specific reference. But if someone copies the YAML and wants a different env, they'd need to edit the path manually. Minor inconsistency.Commit email
ci@pal-e.dev: Not a real domain MX record (presumably). Fine for git attribution but worth noting this is a noreply-style address. No action needed.The token is passed via HTTP URL (
http://ci-token:${FORGEJO_TOKEN}@...): This is the internal cluster URL (port 80), not external HTTPS. The token will appear ingit remote -voutput inside the container. Since this is an ephemeral CI container, the risk is low, but if Woodpecker logsgitcommands verbosely, the token could leak into CI logs. Consider using.netrcorgit credential.helperinstead of URL-embedded credentials (the existing.woodpecker.yamlclone step already uses the.netrcpattern -- see lines 9-10 of the existing file). This would be consistent with the existing pattern in this repo.Missing
westside-contractsoverlay: The YAML template notes "(no overlay yet -- create one first)". This is documented discovered scope. Should have a corresponding Forgejo issue if it doesn't already.SOP COMPLIANCE
204-image-tag-automationreferences issue #204feedback_todos_plan_pipeline.mdfrom_secret, not hardcodedPROCESS OBSERVATIONS
DORA impact:
kubectl set imageor a manual PR to pal-e-deployments. Automating the tag update closes the last gap in the CI-to-ArgoCD pipeline. High-value change.[skip ci]prefix prevents infinite loops. Good defensive design.Dependency chain: This PR depends on #203 (ArgoCD source URLs) for end-to-end verification. The PR body correctly notes this. The 9 per-repo rollout issues are discovered scope that should be tracked.
The DRY violation is the primary concern. The script is good engineering. The YAML template duplicating it creates maintenance burden across 9+ repos that will each copy the inline version. A bug fix would require updating 10 files across 10 repos instead of 1 file in 1 repo.
VERDICT: NOT APPROVED
Two blockers must be addressed:
.shscript OR inline YAML) and have the other reference it. Two independent copies of token-authenticated push-with-retry logic is a divergence risk.latest, dualnewTagentries). A shell script inscripts/tests/that creates mock files and asserts output would suffice.QA Blocker Fixes (
ec3b484)1. DRY violation resolved
woodpecker-update-tag-step.yamlno longer re-implements the clone/sed/push logic inline. It now downloads and executes the canonicalupdate-kustomize-tag.shfrom pal-e-platform's main branch via wget. The shell script remains the single source of truth.2. Test coverage added
New
scripts/test-update-kustomize-tag.sh-- 7 test cases covering:"3ecec95") replacementlatesttag replacementnewTagentries (basketball-api pattern) -- both updatednewTagfields (newName,namespace) left untouchedAll 7 pass.
PR #205 Review (Re-review)
Previous review identified two blockers: DRY violation (YAML template duplicated sed logic inline) and no test coverage. Commit
ec3b484was pushed to address both. This re-review evaluates the fix.DOMAIN REVIEW
Tech stack: POSIX shell scripts + Woodpecker CI YAML. Domain: CI/CD automation, kustomize overlay management.
File paths: All three files are at repo root level (
scripts/update-kustomize-tag.sh,scripts/test-update-kustomize-tag.sh,scripts/woodpecker-update-tag-step.yaml). No worktree path contamination. Clean.DRY fix verified: The YAML template (
woodpecker-update-tag-step.yamllines 28-30) now downloads the canonical script viawgetfrom Forgejo raw URL and runs it withsh. Zero inline sed logic. All business logic lives inupdate-kustomize-tag.sh. The previous blocker is resolved.Shell script quality (
update-kustomize-tag.sh):set -euat top -- good, fails on errors and unset vars${VAR:?msg}pattern -- clean POSIXOVERLAY_ENV,FORGEJO_HOST,DEPLOY_REPO)git clone --depth 5with comment explaining why not depth 1 -- good operational awarenessgit diff --quietbefore committing -- prevents empty commits[skip ci]prefix on commit message -- prevents CI loopssed pattern:
s/^\( newTag:\s*\).*$/\1${IMAGE_TAG}/-- anchored to 2-space indent, matchesnewTag:field specifically. Won't accidentally matchnewName:or top-level fields. Test 7 explicitly validates this boundary.Potential sed injection:
IMAGE_TAGis interpolated directly into the sed expression. In this context,IMAGE_TAGcomes fromCI_COMMIT_SHA(a hex git SHA), so the attack surface is minimal. However, if someone passes a tag containing/or&characters, the sed would break or produce unexpected output. This is a nit given the CI_COMMIT_SHA source, but worth noting.alpine/git:latestimage tag: The template uses an unpinned:latesttag. This is a known tradeoff for reference templates -- consumers should pin to a specific version.BLOCKERS
None. Both previous blockers are resolved:
DRY violation -- FIXED. YAML template delegates to the shell script via
wget+sh. Single source of truth for sed logic.Test coverage -- FIXED.
test-update-kustomize-tag.shprovides 7 test cases:latesttagnewTagentries (basketball-api pattern)newTagfields unchanged (newName,namespace)Covers happy path, edge cases, and boundary conditions. Test harness uses
mktemp,trap cleanup, and proper exit codes.NITS
sed pattern duplication between test and script: The test file duplicates the sed pattern in
run_sed()(line 26) rather than sourcing it from the script. If someone updates the pattern in one file but not the other, tests would pass but production would differ. Consider extracting the sed pattern as a shared function or having the test source the script. Low risk since the pattern is simple, but worth noting for maintainability.alpine/git:latestin template: The reference template uses an unpinned image tag. Consider adding a comment recommending consumers pin to a specific version (e.g.,alpine/git:v2.43.0).IMAGE_TAGnot validated as hex: The script validates thatIMAGE_TAGis non-empty but does not validate it's a valid git SHA (hex chars only). A tag containing sed metacharacters (/,&,\) could produce unexpected sed behavior. Low risk sinceCI_COMMIT_SHAis always hex, but a regex guard likeecho "${IMAGE_TAG}" | grep -qE '^[a-f0-9]+$'would add defense in depth.CLONE_DIRis hardcoded to/tmp/pal-e-deployments: If two pipelines for different overlays run concurrently in the same container, they would collide. Therm -rfat line 41 mitigates this since Woodpecker runs each step in its own container, but a unique suffix (e.g.,mktemp -d) would be safer.SOP COMPLIANCE
204-image-tag-automationreferences issue #204FORGEJO_TOKENsourced from Woodpecker secret, onlyxxxplaceholder in commentsPROCESS OBSERVATIONS
[skip ci]). The sed pattern is well-tested.VERDICT: APPROVED