fix(no-anti-affinity): support topology spread constraints - #1224
Conversation
|
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: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe anti-affinity checker now treats matching ChangesTopology spread support
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The topology spread alternative is implemented and documented consistently with its intended best-effort scheduling behavior. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 3 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/antiaffinity/template_test.go`:
- Around line 135-152: Add a focused test covering TopologySpreadConstraints
with WhenUnsatisfiable set to ScheduleAnyway in the AntiAffinityTestSuite
helpers and related deployment setup. The current
addDeploymentWithTopologySpreadConstraint path only exercises DoNotSchedule, so
extend the test suite with a case that uses ScheduleAnyway and asserts the
intended behavior in topologySpreadConstraintsMatchAgainstNodes and the
deployment validation flow. Use the existing
addDeploymentWithTopologySpreadConstraint, addDeploymentWithReplicas, and
ctx.ModifyDeployment helpers to keep the new case aligned with the current test
structure.
In `@pkg/templates/antiaffinity/template.go`:
- Around line 115-127: The helper topologySpreadConstraintsMatchAgainstNodes
currently treats every matching TopologySpreadConstraint as enforcing spread,
including soft preferences. Update this function to only return true when the
constraint’s WhenUnsatisfiable is DoNotSchedule, in addition to the existing
topologyKeyMatcher and labelSelector match. Keep the LabelSelectorAsSelector
error handling as-is, and add the new guard directly in the conditional so
ScheduleAnyway constraints are ignored.
🪄 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: Enterprise
Run ID: 8c1c8f5e-a8f2-43c2-a656-48d1c2fa212a
⛔ Files ignored due to path filters (1)
docs/generated/checks.mdis excluded by!**/generated/**
📒 Files selected for processing (3)
pkg/builtinchecks/yamls/no-anti-affinity.yamlpkg/templates/antiaffinity/template.gopkg/templates/antiaffinity/template_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1224 +/- ##
===========================================
- Coverage 62.36% 31.45% -30.91%
===========================================
Files 197 239 +42
Lines 4854 6572 +1718
===========================================
- Hits 3027 2067 -960
- Misses 1439 4328 +2889
+ 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:
|
|
@rhybrillou Could you please take look at that? We are currently using the fork version but we would like to add this feature and use the official one |
Co-authored-by: Yann Brillouet <91869377+rhybrillou@users.noreply.github.com>
|
Please adjust the source of the documentation file as well (my second suggestion). |
Tried to do it by using apply suggestion but all I got was |
Fixes #894.
Summary:
Testing: