✨ Fields for placement score visibility feature#428
✨ Fields for placement score visibility feature#428bhperry wants to merge 1 commit intoopen-cluster-management-io:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughAdded score reporting, sorting, and rate-limiting fields: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bhperry The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
bhperry
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
cluster/v1beta1/types_placementdecision.go (1)
61-63: Add+optionalmarker andomitemptyto theScorefield for API consistency.The
Scorefield lacks the+optionalkubebuilder marker. Looking at the corresponding CRD schema,scoreis not in therequiredlist (onlyclusterNameandreasonare required). For consistency with the CRD and API documentation clarity, consider adding the+optionalmarker andomitemptyto the JSON tag.♻️ Proposed fix
- // Score is the computed score for the cluster based on configured prioritizers - Score int64 `json:"score"` + // Score is the computed score for the cluster based on configured prioritizers + // +optional + Score int64 `json:"score,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/v1beta1/types_placementdecision.go` around lines 61 - 63, The Score field in the PlacementDecision type is missing the kubebuilder +optional marker and omitempty JSON tag; update the Score field declaration in types_placementdecision.go by adding the comment marker "+optional" immediately above the Score field and change the struct tag from `json:"score"` to `json:"score,omitempty"` so the generated CRD and JSON serialization match the API schema (field name: Score).cluster/v1beta1/types_placement.go (1)
103-119: Consider adding integration test coverage for the new fields.Based on the relevant code snippets, the existing integration tests in
test/integration/api/placement_test.godo not exercise the newSortBy,ScoreRateLimit, orLastScoreUpdateTimefields. While this PR focuses on API field definitions, test coverage would help validate the schema and prevent regressions.Would you like me to open an issue to track adding test coverage for these new fields?
Also applies to: 185-194, 473-475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/v1beta1/types_placement.go` around lines 103 - 119, The integration tests don't cover the newly added API fields SortBy (type PlacementSortByType), ScoreRateLimit (string), and LastScoreUpdateTime; add tests in test/integration/api/placement_test.go that create/update Placement resources asserting the schema round-trips and behavior: verify SortBy accepts the default and "Score"/"ClusterName" values and is persisted, verify ScoreRateLimit accepts the validated duration pattern and is persisted, and verify LastScoreUpdateTime is set/updated when score-related updates occur; reference PlacementSortByType, SortBy, ScoreRateLimit, and LastScoreUpdateTime in your test cases and include assertions for JSON serialization/deserialization and API response fields to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml`:
- Around line 435-442: The regex for the scoreRateLimit CRD field is wrong
because the character class `[h|m|s]` treats `|` as a literal; update the
pattern for the scoreRateLimit field (the YAML key named scoreRateLimit and its
pattern entry) to use a proper character class without pipes (e.g., `[hms]`) so
the pattern matches numeric values followed by h, m, or s as intended.
- Around line 443-454: The description for the CRD field sortBy contains a
typographical error: change "orderd" to "ordered" in the multi-line description
under the sortBy spec (the block that begins with "SortBy sets the sort order
for decisions." and includes the lines starting "If sortBy is ..."); update the
string so it reads "If sortBy is \"ClusterName\", decisions will be ordered
alphanumerically by cluster name" to correct the typo while leaving the rest of
the text and enum/type definitions unchanged.
In `@cluster/v1beta1/types_placement.go`:
- Around line 103-111: Fix the typo in the comment for the SortBy field: change
"orderd" to "ordered" in the comment that describes the sorting behavior above
the SortBy PlacementSortByType `json:"sortBy,omitempty"` declaration (reference
symbols: SortBy, PlacementSortByType).
- Around line 113-119: The kubebuilder validation Pattern on the ScoreRateLimit
field is wrong because it uses '|' inside a character class; update the
annotation on ScoreRateLimit to use a proper alternation or character class
(e.g. change Pattern="^[0-9]+[h|m|s]$" to Pattern="^[0-9]+(h|m|s)$" or
Pattern="^[0-9]+[hms]$") so only valid duration suffixes are accepted; modify
the struct tag on ScoreRateLimit accordingly and run codegen/validation to
confirm.
---
Nitpick comments:
In `@cluster/v1beta1/types_placement.go`:
- Around line 103-119: The integration tests don't cover the newly added API
fields SortBy (type PlacementSortByType), ScoreRateLimit (string), and
LastScoreUpdateTime; add tests in test/integration/api/placement_test.go that
create/update Placement resources asserting the schema round-trips and behavior:
verify SortBy accepts the default and "Score"/"ClusterName" values and is
persisted, verify ScoreRateLimit accepts the validated duration pattern and is
persisted, and verify LastScoreUpdateTime is set/updated when score-related
updates occur; reference PlacementSortByType, SortBy, ScoreRateLimit, and
LastScoreUpdateTime in your test cases and include assertions for JSON
serialization/deserialization and API response fields to prevent regressions.
In `@cluster/v1beta1/types_placementdecision.go`:
- Around line 61-63: The Score field in the PlacementDecision type is missing
the kubebuilder +optional marker and omitempty JSON tag; update the Score field
declaration in types_placementdecision.go by adding the comment marker
"+optional" immediately above the Score field and change the struct tag from
`json:"score"` to `json:"score,omitempty"` so the generated CRD and JSON
serialization match the API schema (field name: Score).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f18b9bb0-9d2b-43c4-9740-8a31d813a67e
📒 Files selected for processing (5)
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yamlcluster/v1beta1/0000_03_clusters.open-cluster-management.io_placementdecisions.crd.yamlcluster/v1beta1/types_placement.gocluster/v1beta1/types_placementdecision.gocluster/v1beta1/zz_generated.deepcopy.go
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml
Show resolved
Hide resolved
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml
Show resolved
Hide resolved
|
|
||
| // SortBy sets the sort order for decisions. | ||
| // It can be "ClusterName", "Score", or "", where "" is "ClusterName" by default. | ||
| // If sortBy is "ClusterName", decisions will be orderd alphanumerically by cluster name | ||
| // If sortBy is "Score", decisions will be ordered numerically in descending order by score, | ||
| // then by cluster name in the event of a tie | ||
| // +kubebuilder:default:=ClusterName | ||
| // +optional | ||
| SortBy PlacementSortByType `json:"sortBy,omitempty"` |
There was a problem hiding this comment.
Minor typo: "orderd" should be "ordered".
Line 106 contains a typo in the comment.
📝 Proposed fix
// SortBy sets the sort order for decisions.
// It can be "ClusterName", "Score", or "", where "" is "ClusterName" by default.
- // If sortBy is "ClusterName", decisions will be orderd alphanumerically by cluster name
+ // If sortBy is "ClusterName", decisions will be ordered alphanumerically by cluster name
// If sortBy is "Score", decisions will be ordered numerically in descending order by score,
// then by cluster name in the event of a tie📝 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.
| // SortBy sets the sort order for decisions. | |
| // It can be "ClusterName", "Score", or "", where "" is "ClusterName" by default. | |
| // If sortBy is "ClusterName", decisions will be orderd alphanumerically by cluster name | |
| // If sortBy is "Score", decisions will be ordered numerically in descending order by score, | |
| // then by cluster name in the event of a tie | |
| // +kubebuilder:default:=ClusterName | |
| // +optional | |
| SortBy PlacementSortByType `json:"sortBy,omitempty"` | |
| // SortBy sets the sort order for decisions. | |
| // It can be "ClusterName", "Score", or "", where "" is "ClusterName" by default. | |
| // If sortBy is "ClusterName", decisions will be ordered alphanumerically by cluster name | |
| // If sortBy is "Score", decisions will be ordered numerically in descending order by score, | |
| // then by cluster name in the event of a tie | |
| // +kubebuilder:default:=ClusterName | |
| // +optional | |
| SortBy PlacementSortByType `json:"sortBy,omitempty"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cluster/v1beta1/types_placement.go` around lines 103 - 111, Fix the typo in
the comment for the SortBy field: change "orderd" to "ordered" in the comment
that describes the sorting behavior above the SortBy PlacementSortByType
`json:"sortBy,omitempty"` declaration (reference symbols: SortBy,
PlacementSortByType).
8585258 to
e725dde
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cluster/v1beta1/types_placement.go (1)
104-109:⚠️ Potential issue | 🟡 MinorAlign the
SortBydocs with the actual schema.Line 105 says
""is accepted, butPlacementSortByTypeonly allowsClusterNameandScore, so an explicit empty string will still fail CRD validation. I’d document this as “when unset, defaults toClusterName” instead. Line 106 also still has the previously flaggedorderdtypo.📝 Proposed fix
// SortBy sets the sort order for decisions. - // It can be "ClusterName", "Score", or "", where "" is "ClusterName" by default. - // If sortBy is "ClusterName", decisions will be orderd alphanumerically by cluster name + // It can be "ClusterName" or "Score". If unset, "ClusterName" is used by default. + // If sortBy is "ClusterName", decisions will be ordered alphanumerically by cluster name. // If sortBy is "Score", decisions will be ordered numerically in descending order by score, // then by cluster name in the event of a tieVerify the generated schema still only permits the two enum values and does not admit an explicit empty string:
#!/bin/bash set -euo pipefail fd '0000_02_clusters\.open-cluster-management\.io_placements\.crd\.yaml$' -x sh -c ' sed -n "/sortBy:/,/scoreRateLimit:/p" "$1" ' sh {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/v1beta1/types_placement.go` around lines 104 - 109, Update the SortBy field docs to match the schema: remove the claim that an explicit "" is accepted and instead state that when unset it defaults to ClusterName; fix the typo "orderd" to "ordered"; keep the kubebuilder default tag (+kubebuilder:default:=ClusterName) and ensure PlacementSortByType's enum remains only "ClusterName" and "Score" so CRD validation still rejects an explicit empty string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cluster/v1beta1/types_placement.go`:
- Around line 104-109: Update the SortBy field docs to match the schema: remove
the claim that an explicit "" is accepted and instead state that when unset it
defaults to ClusterName; fix the typo "orderd" to "ordered"; keep the
kubebuilder default tag (+kubebuilder:default:=ClusterName) and ensure
PlacementSortByType's enum remains only "ClusterName" and "Score" so CRD
validation still rejects an explicit empty string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a7b2586-fbd9-43ff-aedc-da0a97d8acd2
📒 Files selected for processing (5)
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yamlcluster/v1beta1/0000_03_clusters.open-cluster-management.io_placementdecisions.crd.yamlcluster/v1beta1/types_placement.gocluster/v1beta1/types_placementdecision.gocluster/v1beta1/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cluster/v1beta1/zz_generated.deepcopy.go
- cluster/v1beta1/types_placementdecision.go
- cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml
bhperry
force-pushed
the
placement-decision-score
branch
from
e725dde to
c0cfd43
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cluster/v1beta1/types_placement.go (1)
103-111:⚠️ Potential issue | 🟡 MinorMinor typo: "orderd" should be "ordered".
Line 106 contains a typo in the SortBy field comment.
📝 Proposed fix
// SortBy sets the sort order for decisions. // It can be "ClusterName", or "Score". - // If sortBy is "ClusterName", decisions will be orderd alphanumerically by cluster name + // If sortBy is "ClusterName", decisions will be ordered alphanumerically by cluster name // If sortBy is "Score", decisions will be ordered numerically in descending order by score, // then by cluster name in the event of a tie🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/v1beta1/types_placement.go` around lines 103 - 111, Fix the typo in the comment for the SortBy field: change "orderd" to "ordered" in the docstring above the SortBy PlacementSortByType `json:"sortBy,omitempty"` declaration so the comment correctly reads that decisions will be ordered alphanumerically by cluster name; leave all other wording and annotations intact.cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml (1)
443-454:⚠️ Potential issue | 🟡 MinorMinor typo: "orderd" should be "ordered".
Line 448 contains a typo in the sortBy description that should be fixed.
📝 Proposed fix
- If sortBy is "ClusterName", decisions will be orderd alphanumerically by cluster name + If sortBy is "ClusterName", decisions will be ordered alphanumerically by cluster name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml` around lines 443 - 454, The description for the sortBy field contains a typo ("orderd")—update the description string for the sortBy property (the SortBy description in the CRD for sortBy / enum ClusterName/Score) to replace "orderd" with "ordered" so the sentence reads "decisions will be ordered alphanumerically by cluster name"; leave the rest of the description and enum values unchanged.
🧹 Nitpick comments (1)
cluster/v1beta1/types_placementdecision.go (1)
61-64: Consider addingomitemptyto the Score JSON tag for consistency.The field is marked
+optionalbut the JSON tagjson:"score"lacksomitempty. This means a score of0will always be serialized in the JSON output, even when the score hasn't been computed. If0is a valid score value and should be distinguishable from "no score," the current approach is correct. Otherwise, consider addingomitemptyto avoid always serializing the zero value.♻️ Suggested change if omitempty is desired
// Score is the computed score for the cluster based on configured prioritizers // +optional - Score int64 `json:"score"` + Score int64 `json:"score,omitempty"`Please verify whether
0is a valid score that should be distinguishable from "score not set." If so, the current implementation is correct. If0always means "unset," addingomitemptywould be more appropriate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/v1beta1/types_placementdecision.go` around lines 61 - 64, The Score field in the PlacementDecision struct is marked +optional but its JSON tag is json:"score", so zero (0) will always be serialized; if a zero score should be treated as "unset" update the struct field Score int64 JSON tag to include omitempty (json:"score,omitempty") to avoid emitting zero values, otherwise leave it as-is if 0 is a valid computed score that must be distinguishable from unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml`:
- Around line 443-454: The description for the sortBy field contains a typo
("orderd")—update the description string for the sortBy property (the SortBy
description in the CRD for sortBy / enum ClusterName/Score) to replace "orderd"
with "ordered" so the sentence reads "decisions will be ordered alphanumerically
by cluster name"; leave the rest of the description and enum values unchanged.
In `@cluster/v1beta1/types_placement.go`:
- Around line 103-111: Fix the typo in the comment for the SortBy field: change
"orderd" to "ordered" in the docstring above the SortBy PlacementSortByType
`json:"sortBy,omitempty"` declaration so the comment correctly reads that
decisions will be ordered alphanumerically by cluster name; leave all other
wording and annotations intact.
---
Nitpick comments:
In `@cluster/v1beta1/types_placementdecision.go`:
- Around line 61-64: The Score field in the PlacementDecision struct is marked
+optional but its JSON tag is json:"score", so zero (0) will always be
serialized; if a zero score should be treated as "unset" update the struct field
Score int64 JSON tag to include omitempty (json:"score,omitempty") to avoid
emitting zero values, otherwise leave it as-is if 0 is a valid computed score
that must be distinguishable from unset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf904a82-02c2-44b2-aca4-c55c68dd0932
📒 Files selected for processing (5)
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yamlcluster/v1beta1/0000_03_clusters.open-cluster-management.io_placementdecisions.crd.yamlcluster/v1beta1/types_placement.gocluster/v1beta1/types_placementdecision.gocluster/v1beta1/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cluster/v1beta1/0000_03_clusters.open-cluster-management.io_placementdecisions.crd.yaml
Signed-off-by: Ben Perry <[email protected]>
bhperry
force-pushed
the
placement-decision-score
branch
from
c0cfd43 to
f7b3ac9
Compare
1 participant
Summary
Add fields to support including placement decision score in the status of PlacementDecisions
Related issue(s)
Placement Score Visibility Enhancement
Summary by CodeRabbit