Add cross-pillar review Woodpecker step #63
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
ldraney/pal-e-platform!63
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "62-add-cross-pillar-review-woodpecker-step"
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
Adds a
cross-pillar-reviewpipeline step that automatically creates a Forgejo issue when a merge to main changes files matching cross-pillar trigger patterns. This implements Level 2 of the cross-pillar trigger convention — automated detection replaces manual vigilance.Closes #62
Changes
.woodpecker.yaml— addedcross-pillar-reviewstep afterapply:alpine:3.19(lightweight — only git, curl, jq)applyfailure: ignoreso it never blocks the pipelinegit diff HEAD~1 --name-onlydetects changed files, matches against three trigger patterns (.woodpecker*,terraform/modules/*/main.tf,salt/), and creates a Forgejo issue with matched patterns, changed files list (truncated at 50), and a review checklistforgejo-http.forgejo.svc.cluster.local) matching existing plan step patternPre-requisite
The
forgejo_tokenWoodpecker repo secret currently hasevents: [pull_request]only. It must be updated toevents: [push, pull_request]before this step can access the token on push-to-main pipelines. This can be done via Woodpecker UI ormcp__woodpecker__update_repo_secret.Test Plan
forgejo_tokensecret has push events enabled.woodpecker.yamland confirm a[Cross-Pillar Review]issue is auto-createdterraform/k3s.tfvarsdoes NOT create an issuefailure: ignore)tofu validateneeded — this is a.woodpecker.yaml-only change with no Terraform modificationsReview Checklist
.woodpecker.yamlcode style ($$escaping, environment block, command list)forgejo_tokensecret events updated to includepush(pre-requisite, not in this PR)Related
plan-pal-e-agencyPhase 9 → 9hconvention-cross-pillar-triggersTofu Plan Output
Self-Review
Verdict: LGTM
Checked
yaml.safe_load)failure: ignoreis a valid Woodpecker step-level directive (confirmed in docs)CI_PIPELINE_URLis a valid Woodpecker built-in env var (confirmed in docs)$$escaping matches existing step style (plan step uses same pattern)depends_on: applyensures step runs after deploy|| trueon curl provides defense-in-depth alongsidefailure: ignore.woodpecker*,terraform/modules/*/main.tf,salt/*jq --rawfilefor body (same pattern as plan step's--rawfile) -- avoids heredoc-in-YAML issuesPre-requisite (not in this PR)
The
forgejo_tokenWoodpecker repo secret needsevents: [push, pull_request]-- currently only[pull_request]. Must be updated before this step can access the token on push-to-main pipelines.PR #63 Review
BLOCKERS
1.
git diff HEAD~1will fail on shallow clone (silent no-op)Woodpecker's default clone step uses
--depth=1. With only one commit in history,HEAD~1does not exist, sogit diff HEAD~1 --name-onlywill exit with a fatal error. Becausefailure: ignoreis set, the step will silently fail on every single run without creating any issue. The feature will appear deployed but never actually fire.Fix options (in order of simplicity):
git diff HEAD~1 --name-onlywithgit diff-tree --no-commit-id --name-only -r HEAD-- this command inspects the commit's own tree diff and works correctly in shallow clones.clone:block at the top of.woodpecker.yaml.2.
printf "$$REASONS"interprets%as format specifiersLine 205 (
printf "$$REASONS") will mangle any filename containing%characters. For example, a file namedsalt/100%done.slswould causeprintfto interpret%das an integer format specifier, producing garbled output or an error.Fix: Use
printf '%s' "$$REASONS"instead ofprintf "$$REASONS".NITS
1.
forgejo_tokensecret event scope not enforced by this PRThe PR body correctly documents that
forgejo_tokenmust be updated to includepushevents, but this is a runtime prerequisite that lives outside the diff. If forgotten, the step will silently fail (empty token, 401 from Forgejo API, swallowed by|| true). Consider adding a comment in the YAML near thefrom_secretline to make this dependency visible to future readers.2.
terraform/modules/*/main.tfpattern matches no current filesThe glob
terraform/modules/*/main.tfdoes not match any files in the current repo (there is noterraform/modules/directory -- all.tffiles are atterraform/root). This is presumably forward-looking for when modules are extracted, which is fine, but worth noting that this trigger pattern is currently inert.3. No deduplication guard for repeated merges
If two consecutive merges to main both touch
salt/files, two separate[Cross-Pillar Review]issues will be created. There is no check for an existing open issue with the same title prefix. This may be intentional (each merge deserves its own review), but could generate noise during high-activity periods. A comment in the YAML noting this design decision would help future readers.4.
curl -shides HTTP errorsThe
curlcall uses-s(silent) and|| true, so a 4xx/5xx response from Forgejo will be silently swallowed. Consider adding-f(fail on HTTP errors) or logging the response body so that pipeline logs contain diagnostic information when the step does fire but fails to create the issue.SOP COMPLIANCE
62-add-cross-pillar-review-woodpecker-stepreferences #62)plan-pal-e-agencyPhase 9 -> 9h)Closes #62present.envfiles committed.woodpecker.yamlonly)tofu plan/tofu validatenot requiredconvention-cross-pillar-triggersLevel 2) -- cannot verify issue format against convention spec (pal-e-docs not accessible to QA agent), but the issue template includes trigger patterns, changed files, and a review checklist which appears reasonableVERDICT: NOT APPROVED
Two blockers must be fixed before merge:
git diff HEAD~1withgit diff-tree --no-commit-id --name-only -r HEAD.printfformat string injection is a correctness bug for filenames containing%.QA Blockers Fixed
Both blockers from the QA review have been resolved in commit
4fc73bb:Shallow clone fix -- Replaced
git diff HEAD~1 --name-onlywithgit diff-tree --no-commit-id --name-only -r HEAD. Thediff-treecommand inspects the commit's own tree diff and works correctly in Woodpecker's default--depth=1shallow clones.printf format string injection -- Changed
printf "$$REASONS"toprintf '%s' "$$REASONS". Filenames containing%characters (e.g.,salt/100%done.sls) will no longer be misinterpreted as format specifiers.PR #63 Re-Review
Re-review after fixes for the two blockers identified in the previous review.
BLOCKER FIXES VERIFIED
1. Shallow clone fix (line 141)
git diff HEAD~1 --name-onlyhas been replaced withgit diff-tree --no-commit-id --name-only -r HEAD. This is correct --diff-treeoperates on the commit object directly and does not require parent commit history to be present, so it works in shallow clones (Woodpecker's default). The flags are appropriate:--no-commit-idsuppresses the SHA line,--name-onlyoutputs just file paths,-rrecurses into subtrees.2. Printf injection fix (line 205)
printf "$$REASONS"has been replaced withprintf '%s' "$$REASONS". This is correct -- the%sformat specifier ensures the variable contents are treated as a literal string, not as a format string. Any%characters in filenames or constructed text will not be interpreted as printf conversion specifiers.Both fixes are surgical -- no other lines were changed, and no regressions were introduced.
BLOCKERS
None.
NITS
echo "$$CHANGED_LIST"(line 209) -- This variable is constructed with literal\nsequences (lines 187, 189). In Alpine's busyboxecho,\nis interpreted as a newline, which happens to be the desired behavior here for formatting. Worth a comment noting this relies on busybox echo behavior, but non-blocking since the image is pinned toalpine:3.19.SOP COMPLIANCE
62-add-cross-pillar-review-woodpecker-stepmatches issue #62)plan-pal-e-agencyPhase 9 -> 9h)from_secretreferences, no plaintext credentials)Closes #62present in PR bodytofu plan/tofu validatenot requiredVERDICT: APPROVED