openshift/v4_23_exp validate: report CEX kernel arg error once - #732
openshift/v4_23_exp validate: report CEX kernel arg error once#732deepak0x wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughWalkthrough
ChangesCEX validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change makes the validator report the existing CEX kernel-argument error once instead of twice, with no intended behavior or error-path change. No actionable merge-blocking risk remains after normal checks and review. 🚥 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.
🧹 Nitpick comments (1)
config/openshift/v4_23_exp/validate_test.go (1)
233-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd positive and edge cases to the validation table.
The new row checks only the missing-argument case. Add one row with both CEX sources enabled and
rd.luks.key=/etc/luks/cex.keypresent. Add one edge row with a non-rootstorage.luksCEX entry. These rows verify the valid path and theName == "root"boundary.As per coding guidelines,
**/*_test.gofiles must use table-driven tests with positive, negative, and edge cases.🤖 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 `@config/openshift/v4_23_exp/validate_test.go` around lines 233 - 268, Extend the validation table around the existing CEX missing-kernel-argument case with a positive row where both CEX sources are enabled and KernelArguments contains rd.luks.key=/etc/luks/cex.key, expecting no error. Add an edge-case row with a CEX-enabled storage.luks entry whose Name is not "root", verifying the root-specific validation boundary and expected result.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@config/openshift/v4_23_exp/validate_test.go`:
- Around line 233-268: Extend the validation table around the existing CEX
missing-kernel-argument case with a positive row where both CEX sources are
enabled and KernelArguments contains rd.luks.key=/etc/luks/cex.key, expecting no
error. Add an edge-case row with a CEX-enabled storage.luks entry whose Name is
not "root", verifying the root-specific validation boundary and expected result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bb3318bc-1b17-4ae4-a399-c73ab382bc2c
📒 Files selected for processing (2)
config/openshift/v4_23_exp/validate.goconfig/openshift/v4_23_exp/validate_test.go
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 a single time. Fixes coreos#731 Signed-off-by: Deepak Bhagat <deepak988088@gmail.com>
effc182 to
ec3c4fd
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…rg validation Exercise the valid path (CEX enabled with the kernel argument present) and the root-only boundary (non-root CEX luks entry) so the table covers positive, negative, and edge cases. Signed-off-by: Deepak Bhagat <deepak988088@gmail.com>
|
Superseded by coreos/ignition#2294, which ports this fix now that butane lives in the ignition repo. Closing in favor of the ignition PR that fixes coreos/ignition#2285. |
1 participant
Summary
Config.Validateinconfig/openshift/v4_23_exp/validate.goemittedErrMissingKernelArgumentCextwice at$.openshift.kernel_argumentswhen bothboot_device.luks.cex.enabled: trueand astorage.luksroot entry withcex.enabled: truewere set.Two independent
if-blocks each calledr.AddOnError(c.Append("openshift", "kernel_arguments"), common.ErrMissingKernelArgumentCex)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../test(gofmt, go vet, go test./..., doc validation) passes.Fixes #731
Summary by CodeRabbit
Bug Fixes
Tests