Add check-gen target and fix check-codegen CI to catch stale helm - #130
Add check-gen target and fix check-codegen CI to catch stale helm#130stefanhipfel wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe Makefile patches generated Helm output and adds a ChangesCode generation checks
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/check-codegen.yml:
- Around line 18-19: Update the failure hint in the code generation check
workflow to recommend `make check-gen`, matching the command executed by the
“Run code gen checks” step. Replace the outdated incomplete command while
preserving the surrounding failure guidance.
- Around line 18-19: Update the workflow’s actions/checkout step to set
persist-credentials to false before the Run code gen checks step, preserving the
existing checkout behavior and avoiding persisted credentials for PR-controlled
code.
In `@Makefile`:
- Around line 120-121: Update the check-gen target so generate, manifests, docs,
helm, and fmt execute in a guaranteed sequential order rather than as
independent parallel prerequisites. Preserve the existing subtargets and
check-gen purpose while encoding the ordering explicitly in the Makefile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46c379d3-4953-43ea-8c39-8a20fef369e4
📒 Files selected for processing (2)
.github/workflows/check-codegen.ymlMakefile
| - name: Run code gen checks | ||
| run: make check-gen |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the failure hint to use make check-gen.
The new target also runs manifests and formatting checks, but the message below still recommends the old incomplete command. Point contributors to the exact command that CI now executes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/check-codegen.yml around lines 18 - 19, Update the failure
hint in the code generation check workflow to recommend `make check-gen`,
matching the command executed by the “Run code gen checks” step. Replace the
outdated incomplete command while preserving the surrounding failure guidance.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable persisted checkout credentials before running PR-controlled code.
make check-gen executes the repository’s Makefile. Since this is a pull-request check, a malicious change could read the token persisted by actions/checkout and exfiltrate it. Add persist-credentials: false to the checkout step; no later step appears to require authenticated Git operations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/check-codegen.yml around lines 18 - 19, Update the
workflow’s actions/checkout step to set persist-credentials to false before the
Run code gen checks step, preserving the existing checkout behavior and avoiding
persisted credentials for PR-controlled code.
Source: Linters/SAST tools
| .PHONY: check-gen | ||
| check-gen: generate manifests docs helm fmt ## Run code generation, manifests, docs, helm, and formatting. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Serialize the generation steps for parallel-safe execution.
These are independent prerequisites, not a guaranteed chain. Under make -j check-gen, fmt can run while generators modify files, producing races or nondeterministic diffs. Run the subtargets sequentially or encode their dependencies explicitly.
Proposed fix
.PHONY: check-gen
-check-gen: generate manifests docs helm fmt ## Run code generation, manifests, docs, helm, and formatting.
+check-gen: ## Run code generation, manifests, docs, helm, and formatting.
+ $(MAKE) generate
+ $(MAKE) manifests
+ $(MAKE) docs
+ $(MAKE) helm
+ $(MAKE) fmt📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .PHONY: check-gen | |
| check-gen: generate manifests docs helm fmt ## Run code generation, manifests, docs, helm, and formatting. | |
| .PHONY: check-gen | |
| check-gen: ## Run code generation, manifests, docs, helm, and formatting. | |
| $(MAKE) generate | |
| $(MAKE) manifests | |
| $(MAKE) docs | |
| $(MAKE) helm | |
| $(MAKE) fmt |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 120 - 121, Update the check-gen target so generate,
manifests, docs, helm, and fmt execute in a guaranteed sequential order rather
than as independent parallel prerequisites. Preserve the existing subtargets and
check-gen purpose while encoding the ordering explicitly in the Makefile.
9092b86 to
992ab99
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/check-codegen.yml:
- Around line 11-19: Add workflow-level read-only permissions to the code
generation workflow by setting contents access to read near the workflow
metadata, before jobs such as the existing “Run code gen checks” steps. Keep the
checkout, Go setup, and make check-gen behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 75e7be12-0d3f-4a92-a23b-cdd2a44e1f28
📒 Files selected for processing (2)
.github/workflows/check-codegen.ymlMakefile
| name: Run code generation checks | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v7 | ||
| - uses: actions/setup-go@v7 | ||
| with: | ||
| go-version-file: 'go.mod' | ||
| - name: Run make generate | ||
| run: make generate | ||
| - name: Run code gen checks | ||
| run: make check-gen |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== workflow file ==="
cat -n .github/workflows/check-codegen.yml
echo "=== workflow refs to permissions / pull_request / branches ==="
rg -n "permission|workflow_dispatch|pull_request|branches:|pull_request_target|make check-gen|actions/checkout|actions/setup-go" .github/workflows/check-codegen.yml .github/workflows || true
echo "=== other workflows with explicit permissions for reference ==="
python3 - <<'PY'
from pathlib import Path
for p in sorted(Path(".github/workflows").glob("*.yml")) + sorted(Path(".github/workflows").glob("*.yaml")):
text=p.read_text()
print("\n---", p)
for i,line in enumerate(text.splitlines(),1):
if "permissions:" in line or "pull_request" in line or "workflow_dispatch" in line:
print(f"{i}: {line}")
PYRepository: ironcore-dev/metal-maintenance-operator
Length of output: 6467
Security Misconfiguration (CWE-732): Incorrect Permission Assignment for Critical Resource
Reachability: External
Set explicit read-only workflow permissions.
This pull_request workflow runs untrusted branch code, including a run step in the checked-out source. Add permissions: contents: read at workflow scope so the GITHUB_TOKEN does not rely on repository defaults.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 11-27: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 zizmor (1.28.0)
[warning] 14-14: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/check-codegen.yml around lines 11 - 19, Add workflow-level
read-only permissions to the code generation workflow by setting contents access
to read near the workflow metadata, before jobs such as the existing “Run code
gen checks” steps. Keep the checkout, Go setup, and make check-gen behavior
unchanged.
Source: Linters/SAST tools
- Add check-gen Makefile target (generate manifests docs helm fmt) - Update check-codegen.yml to use check-gen - Add patches to helm target to re-apply POD_NAMESPACE env and ports templating after kubebuilder regenerates manager.yaml - Sync dist/chart/templates/manager/manager.yaml with make helm output Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
992ab99 to
afec5f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Makefile (1)
120-121: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSerialize
check-genprerequisites; parallel runs remain unsafe.
check-genstill listsgenerate manifests docs helm fmtas parallel prerequisites. Undermake -j check-gen,fmtcan run while a generator still modifies files, producing races or nondeterministic diffs. This was flagged in a prior review and is unresolved in this code.Proposed fix
.PHONY: check-gen -check-gen: generate manifests docs helm fmt ## Run code generation, manifests, docs, helm, and formatting. +check-gen: ## Run code generation, manifests, docs, helm, and formatting. + $(MAKE) generate + $(MAKE) manifests + $(MAKE) docs + $(MAKE) helm + $(MAKE) fmt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 120 - 121, Update the check-gen target so generate, manifests, docs, helm, and fmt execute in the required serialized order rather than as parallel prerequisites, while preserving the target’s existing behavior and description.
🧹 Nitpick comments (1)
Makefile (1)
285-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the embedded
python3/sedpatches into a dedicated script.Both patches embed multi-layer escaping directly in the Makefile: Make's
$$, the shell's\$, and Python single-quoted strings all interact in the same line (for example\$$kon line 292). Auditing the escaping correctly, as this review had to do, requires tracing all three layers. A dedicatedhack/patch-helm-chart.py(or similar) script, invoked from thehelmtarget, would keep the same guarded, fail-loud behavior while being easier to read, lint, and unit test, and would have caught thesed/BSD portability issue directly during review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 285 - 302, Extract the embedded Python and sed logic from the helm target’s manager environment and ports patch blocks into a dedicated script such as hack/patch-helm-chart.py. Have the Makefile invoke that script while preserving the existing idempotent guards, validation failures, and generated manager.yaml output, including POD_NAMESPACE, env/envOverrides, and values-driven ports handling. Ensure the script avoids non-portable sed behavior and keeps the patch logic readable and testable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 296-302: Make the ports patch in the Makefile portable across GNU
and BSD sed by replacing the current multiline sed replacement in the manager
ports patch block with Python, pre-expanded newline handling, or another
portable method. Preserve the existing Values-driven template content and keep
the post-patch validation that reports “ports patch not applied.”
---
Duplicate comments:
In `@Makefile`:
- Around line 120-121: Update the check-gen target so generate, manifests, docs,
helm, and fmt execute in the required serialized order rather than as parallel
prerequisites, while preserving the target’s existing behavior and description.
---
Nitpick comments:
In `@Makefile`:
- Around line 285-302: Extract the embedded Python and sed logic from the helm
target’s manager environment and ports patch blocks into a dedicated script such
as hack/patch-helm-chart.py. Have the Makefile invoke that script while
preserving the existing idempotent guards, validation failures, and generated
manager.yaml output, including POD_NAMESPACE, env/envOverrides, and
values-driven ports handling. Ensure the script avoids non-portable sed behavior
and keeps the patch logic readable and testable.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6703eec2-4ef3-40c4-8f5f-7cd068fc87f7
⛔ Files ignored due to path filters (1)
dist/chart/templates/manager/manager.yamlis excluded by!**/dist/**
📒 Files selected for processing (2)
.github/workflows/check-codegen.ymlMakefile
| @# Patch manager ports: replace static [] with values-driven template | ||
| @grep -qF ' {{- with .Values.manager.ports }}' \ | ||
| dist/chart/templates/manager/manager.yaml || { \ | ||
| sed -i.bak 's/^ ports: \[\]$$/ ports:\n {{- with .Values.manager.ports }}\n {{- toYaml . | nindent 8 }}\n {{- else }}\n []\n {{- end }}/' \ | ||
| dist/chart/templates/manager/manager.yaml && rm -f dist/chart/templates/manager/manager.yaml.bak; \ | ||
| grep -qF ' {{- with .Values.manager.ports }}' dist/chart/templates/manager/manager.yaml || { echo 'ports patch not applied' >&2; exit 1; }; \ | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Makefile context =="
sed -n '280,308p' Makefile | cat -n
echo
echo "== Tooling declarations/available sed versions =="
git ls-files | rg '(^|^/)(Makefile|docker-compose|Dockerfile|Taskfile|\.github/workflows/.*\.ya?ml|README\.)' || true
printf '\nGNU sed -V present? '
(command -v g sed >/dev/null 2>&1 && g sed --version | head -1) || printf 'no\n'
printf 'sed version: '
(command -v sed >/dev/null 2>&1 && sed --version 2>&1 | head -1 || true) || true
echo
echo "== Static sed behavior probe on GNU sed (if present) =="
python3 - <<'PY'
from pathlib import Path
import subprocess, tempfile, textwrap, sys, os
sample = textwrap.dedent("""\
a b
c: []
d: []
z
""")
old = subprocess.run(['sed', '--version'], capture_output=True, text=True)
print("sed_version", old.stdout.splitlines()[0] if old.returncode == 0 else "n/a")
cmd = ["sed", "-e", "s/.*c: \\[\\].*/NEWLINE:/", "-e", "s/.*d: \\[\\].*/NEWLINE:/", "-"]
p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True)
out, err = p.communicate(sample)
print("posix_imitation_probe: " + repr(out))
# Actual literal sed expression that the Makefile uses for sed,
# except remove -i.bak and trailing file so the script can see the command.
sed_expr = r"s/^ ports: \[\]$$/ ports:\n {{- with .Values.manager.ports }}\n {{- toYaml . | nindent 8 }}\n {{- else }}\n []\n {{- end }}/"
p = subprocess.Popen(['sed', '-e', sed_expr], stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True)
out, err = p.communicate(" ports: []\n")
print("ports_patch_literal_output:")
print(out)
print("contains guard marker:", " {{- with .Values.manager.ports }}" in out)
PYRepository: ironcore-dev/metal-maintenance-operator
Length of output: 3415
🌐 Web query:
POSIX sed replacement string newline \n not supported BSD sed
💡 Result:
In POSIX-compliant sed, the behavior of the "\n" sequence within the replacement string of an 's' command is not explicitly defined as a newline character [1][2][3]. While GNU sed supports "\n" as a newline character in replacement strings, BSD sed (including the version on macOS) does not; it treats the literal characters '' and 'n' as such [4][5][6]. To insert a newline in a replacement string using BSD sed, you must use one of the following methods: 1. Literal Newline: Use a backslash followed by a literal newline character [4][7][8]. In a shell command, you can typically hit Enter after the backslash [9][10]: sed 's/pattern/\ replacement/' 2. ANSI C-Quoted String: If your shell supports it (e.g., bash, zsh, ksh), use ANSI C-quoting ($'...') to expand the newline before it reaches sed [4][5][10]: sed 's/pattern/'$'\n''/' These methods are necessary because BSD sed documentation explicitly states that to specify a newline character in the replacement string, it must be preceded by a backslash [7][8]. Note that while "\n" is not supported in the replacement string, it is typically recognized as a newline character within the regular expression portion of the command across both GNU and BSD sed implementations [11][5].
Citations:
- 1: https://pubs.opengroup.org/onlinepubs/9799919799/utilities/sed.html
- 2: https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/utilities/sed.html
- 3: https://man.archlinux.org/man/sed.1p
- 4: https://stackoverflow.com/questions/24275070/sed-not-giving-me-correct-substitute-operation-for-newline-with-mac-difference
- 5: https://unix.stackexchange.com/questions/42321/how-can-i-instruct-bsd-sed-to-interpret-escape-sequences-like-n-and-t
- 6: https://www.baeldung.com/linux/gnu-bsd-stream-editor
- 7: https://man.bsd.lv/FreeBSD-11.2/sed.1
- 8: https://man.netbsd.org/NetBSD-9.3/sed.1
- 9: https://unix.stackexchange.com/questions/140763/replace-n-by-a-newline-in-sed-portably
- 10: https://stackoverflow.com/questions/1421478/how-do-i-use-a-new-line-replacement-in-a-bsd-sed
- 11: https://man.openbsd.org/sed.1
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
try:
text = Path('.github/workflows/check-codegen.yml').read_text()
except Exception as e:
print(f"could not read .github/workflows/check-codegen.yml: {e}")
raise SystemExit
for i, line in enumerate(text.splitlines(), 1):
if 'make helm' in line or 'make check-gen' in line or 'checks:' in line:
start = max(1, i-8)
end = min(len(text.splitlines()), i+30)
lines = text.splitlines()
print(f"\n-- .github/workflows/check-codegen.yml:{start}-{end} --")
for n in range(start, end+1):
print(f"{n:4}: {lines[n-1]}")
PYRepository: ironcore-dev/metal-maintenance-operator
Length of output: 1500
Make the ports template patch portable to macOS BSD sed.
This sed replacement uses \n to insert multiple lines. CI uses GNU sed, but macOS uses BSD sed, where \n is not interpreted as a newline in replacement strings. Use Python, Python-style \n expansion before sed, or another portable newline method so make helm and make check-gen do not fail on ports patch not applied.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 296 - 302, Make the ports patch in the Makefile
portable across GNU and BSD sed by replacing the current multiline sed
replacement in the manager ports patch block with Python, pre-expanded newline
handling, or another portable method. Preserve the existing Values-driven
template content and keep the post-patch validation that reports “ports patch
not applied.”
| .PHONY: helm | ||
| helm: manifests kubebuilder | ||
| "$(KUBEBUILDER)" edit --plugins=helm/v2-alpha | ||
| @# Patch manager env: replace generated env block with POD_NAMESPACE fieldRef + env/envOverrides |
There was a problem hiding this comment.
this is handled by #133 and could go here, leaving only the check-gen in place.
2 participants
Adds a
check-genMakefile target (generate manifests docs helm fmt) and updatescheck-codegen.ymlto use it, so PRs that forget to runmake helmare caught in CI.Depends on #131.
Same setup as in metal-operator!
Signed-off-by: Stefan Hipfel stefan.hipfel@sap.com
Summary by CodeRabbit