Repository navigation
butane: report CEX kernel arg error once when both sources set - #2294
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: coreos/ignition/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (9)
📝 WalkthroughWalkthroughOpenShift 4.23 validation now combines boot-device and root-storage CEX settings before checking for the required kernel argument. A regression test checks for one error, and the release notes document the change. ChangesCEX validation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change removes duplicate CEX validation errors without weakening the required kernel-argument check. No actionable merge-blocking risk remains, subject to normal checks. 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Binary size report (
|
| Size | |
|---|---|
Base (main) |
33MiB |
| PR (#2294) | 33MiB |
| Delta | +80B (0.00%) |
|
Hmm, it looks like we have some failures, the tmt tests are fixed if you re-base, however the go tests are actual linting issues. |
|
Done — the lint issue was staticcheck QF1007 on |
b210c8e to
c1f29c8
Compare
|
Rebased onto main to clear the merge conflict in docs/release-notes.md (kept both entries). The staticcheck QF1007 fix rides on top, force-pushed as c1f29c8. |
|
LGTM, thanks for the fix (and apologies for the delay). There is a large number of PRs right now, and its causing alot of pain with release notes. Sorry about that, we will try and reduce that soon. Another rebase will be needed once Pending pr lands. |
When both boot_device.luks.cex.enabled and a storage.luks root entry with cex.enabled are set, the missing 'rd.luks.key=/etc/luks/cex.key' kernel argument was reported twice at $.openshift.kernel_arguments. Consolidate the two independent checks into a single CEX-enabled flag and emit the error exactly once. Behavior and error path are unchanged. Fixes coreos#2285 Signed-off-by: Deepak Bhagat <[email protected]>
The separate cex := false / if ... { cex = true } pattern triggers the
staticcheck QF1007 'could merge conditional assignment into variable
declaration' lint. Replace with a direct initialization from the first
condition.
Signed-off-by: Deepak Bhagat <[email protected]>
c1f29c8 to
8c92754
Compare
Summary
Config.Validateinbutane/config/openshift/v4_23_exp/validate.goreportedErrMissingKernelArgumentCextwice at$.openshift.kernel_argumentswhen bothboot_device.luks.cex.enabled: trueand astorage.luksroot entry withcex.enabled: truewere set.Two independent
if-blocks each calledr.AddOnError(...)when therd.luks.key=/etc/luks/cex.keykernel argument was absent, so both fired and the identical error was reported twice.Fix
Consolidate the two checks into a single
cexflag (set when either the boot-device CEX is enabled or any rootstorage.luksentry has CEX enabled) and emit the error exactly once when the flag is set and the kernel argument is missing. Behavior and error path are unchanged.Test plan
TestValidateConfigcase that sets both CEX sources and an empty kernel argument list, asserting exactly oneErrMissingKernelArgumentCexat$.openshift.kernel_arguments.go test ./butane/config/openshift/v4_23_exp/...passes.Fixes #2285
This PR ports the fix originally opened as coreos/butane#732 for coreos/butane#731, before butane was merged into ignition. It supersedes that PR and closes coreos/butane#731.
cc @prestist