test(memoryrequirements): add regression test for init+regular containers with limits set - #1232
test(memoryrequirements): add regression test for init+regular containers with limits set#1232pujitha24 wants to merge 2 commits into
Conversation
…ners with limits set Motivation: Issue stackrox#428 reported that unset-memory-requirements produced a false positive on a Deployment with both initContainers and containers when resources were set on both. Reproducing this against current main with a hand-built kube-linter binary and a Deployment YAML containing an initContainer and a regular container, each with memory/cpu requests and limits set, shows the check now correctly reports no violations. The fix landed via earlier work (PodSpec.AllContainers(), used by util.PerContainerCheck) that iterates init, regular, and ephemeral containers while checking each container's own Resources field independently. There was, however, no test locking in this behavior, so the scenario could regress silently. Approach: - Add memoryrequirements/template_test.go using the repo's templates.TemplateTestSuite, covering: - an init container and a regular container both with memory requests/limits set -> no diagnostics (the exact stackrox#428 scenario). - an init container missing resources alongside a regular container that has them set -> the init container is still correctly flagged by name, proving the check isn't vacuously passing. - Add AddInitContainerToDeployment to pkg/lintcontext/mocks/container.go, mirroring the existing AddContainerToDeployment helper, since no mock helper previously existed for populating a Deployment's init containers in tests. - Extract the "memory-requirements" string literal into a templateKey const in template.go (matching the convention already used by sibling templates such as latesttag) so the new test can reference it via s.Init(templateKey). Validation: - go build ./... - go test ./pkg/templates/memoryrequirements/... ./pkg/lintcontext/mocks/... (pass) - go test ./... (all packages pass) - golangci-lint run ./pkg/templates/memoryrequirements/... ./pkg/lintcontext/mocks/... (clean) Impact: this is test-only coverage, not a functional fix. The unset-memory-requirements/unset-cpu-requirements checks already behave correctly on current main for the reported scenario; user-visible behavior is unchanged. The benefit is a regression guard preventing this specific false positive from silently reappearing. Fixes stackrox#428 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe memory requirements template now uses a package-level key constant. Mock deployment helpers support adding init containers, and regression tests cover memory limits on both init and regular containers. ChangesMemory requirements validation
Estimated code review effort: 2 (Simple) | ~10 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: 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 `@pkg/templates/memoryrequirements/template_test.go`:
- Around line 54-58: Update the test cases passed to Validate in the memory
requirements template test so request handling is covered alongside limit
handling: either change the existing RequirementsType to "any" or add a separate
"request" case while retaining limit coverage.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d29b887-d360-4cc9-9f98-0aad80592919
📒 Files selected for processing (3)
pkg/lintcontext/mocks/container.gopkg/templates/memoryrequirements/template.gopkg/templates/memoryrequirements/template_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1232 +/- ##
===========================================
- Coverage 62.36% 31.72% -30.64%
===========================================
Files 197 239 +42
Lines 4854 6565 +1711
===========================================
- Hits 3027 2083 -944
- Misses 1439 4305 +2866
+ Partials 388 177 -211
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…krox#428 regression case CodeRabbit noted that TestInitAndRegularContainersWithLimitsSet builds containers with both memory Requests and Limits set, but only validated with RequirementsType "limit", leaving request handling untested. Switch to "any" so the test exercises both code paths. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
Good catch — the fixture set both Re the failing |
Motivation:
Issue #428 reported that unset-memory-requirements produced a false
positive on a Deployment with both initContainers and containers when
resources were set on both. Reproducing this against current main with
a hand-built kube-linter binary and a Deployment YAML containing an
initContainer and a regular container, each with memory/cpu requests
and limits set, shows the check now correctly reports no violations.
The fix landed via earlier work (PodSpec.AllContainers(), used by
util.PerContainerCheck) that iterates init, regular, and ephemeral
containers while checking each container's own Resources field
independently. There was, however, no test locking in this behavior,
so the scenario could regress silently.
Approach:
templates.TemplateTestSuite, covering:
requests/limits set -> no diagnostics (the exact [BUG] unset-memory-requirements validation broken #428 scenario).
that has them set -> the init container is still correctly
flagged by name, proving the check isn't vacuously passing.
mirroring the existing AddContainerToDeployment helper, since no mock
helper previously existed for populating a Deployment's init
containers in tests.
const in template.go (matching the convention already used by
sibling templates such as latesttag) so the new test can reference it
via s.Init(templateKey).
Validation:
Impact: this is test-only coverage, not a functional fix. The
unset-memory-requirements/unset-cpu-requirements checks already behave
correctly on current main for the reported scenario; user-visible
behavior is unchanged. The benefit is a regression guard preventing
this specific false positive from silently reappearing.
Fixes #428
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com