Repository navigation
feat(helm): render optional STAGE_FILTER env on executors - #117
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
PR #117 — feat(helm): render optional STAGE_FILTER env on executors
Must Fix
No blocking findings.
Should Fix
No should-fix findings.
Nice to Have
No nit findings.
Notes
- No helm test harness in this repo. The
helm/directory contains notests/subdirectory and no helm-unittest configuration. CI (.github/workflows/ci.yml) runs onlymake precommit, which has nohelm lintorhelm templatestep. Render verification is documented as a manualhelm template+grepcheck in the (.reviewignore-excluded) dark-factory prompt. This is pre-existing repo-wide debt shared with the precedent PR #116 (CLUSTER/PUBLISH_UNTAGGEDenv); the diff introduces no new test gap, so it does not block.
Traceability Report (Selector Mode)
| Category | Count |
|---|---|
| Candidate findings from rule index | 0 |
| Applicable (matched triggers + relevant) | 0 |
| Skipped (rule-derived, no match) | 0 |
Excluded by .reviewignore |
1 (prompts/in-progress/240-helm-executor-stage-filter-env.md) |
| Mechanical funnel preflight (ast-grep) | 0 findings across 79 YAMLs |
| precommit | skipped (selector mode) — CI covers lint+test |
{
"verdict": "approve",
"summary": "Render logic is sound: hasKey+kindIs(\"bool\") correctly distinguishes absent from false on both the singular and per-entry paths; = reassignment is valid Go template syntax with no blank-line artefacts; per-entry hasKey takes precedence over inherited value; no helm test harness or CI step exists but is pre-existing debt shared with PR #116, not introduced here.",
"comments": [],
"concerns_addressed": [
{
"concern": "correctness — hasKey + kindIs(\"bool\") on singular executor: verify condition handles all edge cases (non-bool values render nothing)",
"disposition": "not-an-issue",
"detail": "hasKey checks the key exists in the map; kindIs(\"bool\") gates on a real YAML boolean. String 'false' or any non-bool yields false on kindIs → nothing rendered. Commented-out default in values.yaml means unset → hasKey false → nothing rendered by default. Identical pattern used for PUBLISH_UNTAGGED in controller-statefulset.yaml (PR #116 precedent)."
},
{
"concern": "correctness — executors[]: $stageFilter reassignment uses = not :=; verify no stray blank lines in env list when stageFilter is inherited vs per-entry",
"disposition": "not-an-issue",
"detail": "Go text/template supports = reassignment to an existing outer-scope variable (Go 1.11+). All four added lines (272–275) use {{- chomp-left, collapsing the action-only lines into whitespace-only source lines that produce no output. No blank-line artefact in either branch (inherited-unset / per-entry false)."
},
{
"concern": "correctness — per-entry precedence: hasKey $e \"stageFilter\" must correctly override inherited $.Values.executor.stageFilter; verify false from per-entry is not confused with inherited-unset",
"disposition": "not-an-issue",
"detail": "hasKey $e \"stageFilter\" returns true for any per-entry key (bool or otherwise). If true and value is bool false → $stageFilter = false → kindIs(\"bool\") true → renders value: \"false\". Helm replaces lists wholesale so per-entry keys are never merged with chart defaults; inherited-unset case correctly yields hasKey false → no override → inherited nil → kindIs(\"bool\") false → nothing rendered."
},
{
"concern": "tests — PR description mentions render checks run locally but no automated helm test coverage is visible in the diff; confirm chart has existing tests or that render checks are part of CI",
"disposition": "not-an-issue",
"detail": "Verified: helm/ has no tests/ directory and no helm-unittest config. .github/workflows/ci.yml runs only make precommit (format+generate+test+check+addlicense). No helm lint or helm template step exists. This is pre-existing repo-wide debt, identical to PR #116 which added CLUSTER/PUBLISH_UNTAGGED env with the same pattern. The diff introduces no new test gap."
}
]
}
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
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.
Adds
executor.stageFilterand per-entryexecutors[].stageFilter, which falls back toexecutor.stageFilter. Either one rendersSTAGE_FILTER(agent-task-executor >= v0.21.0) only when set, sofalserenders and leaving it unset renders nothing. This is for sun, whose executors should ignore taskstage:. Nuke dev and nuke prod leave it unset, so the default stays true. Chart 0.8.0 → 0.9.0.Render checks, run locally because dark-factory prompt 240 ended
partialwith no helm in its container:STAGE_FILTER.executor.stageFilter=false: renders"false"on the singular executor.executors[]list: both Deployments render, and only the entry withstageFilter: falsecarries it.truewith an entry override offalse: renderstrueandfalserespectively.helm lintpasses.The run's unrelated edits (license header stripped from mocks.go, three spec status flips) were reverted before commit.