Refactor role validation logic, remove redundant for loop steps#880
Refactor role validation logic, remove redundant for loop steps#880nalan2012 wants to merge 1 commit intovolcano-sh:mainfrom
Conversation
Signed-off-by: shuguansheng <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the validateGangPolicy function to use a map for role lookups, which improves efficiency by eliminating a nested loop. However, a critical compilation error was introduced where the code attempts to access an undefined variable roleMap instead of the correctly initialized roleNames map.
| for roleName, minReplicas := range minRoleReplicas { | ||
| // Check if the role exists | ||
| if !roleNames[roleName] { | ||
| role, ok := roleMap[roleName] |
There was a problem hiding this comment.
The variable roleMap is undefined in this scope. It should be roleNames to match the map initialized on line 212. This will cause a compilation error.
| role, ok := roleMap[roleName] | |
| role, ok := roleNames[roleName] |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors gangPolicy role validation to avoid repeatedly scanning roles when validating minRoleReplicas.
Changes:
- Build a map of roles keyed by role name for O(1) lookup.
- Remove the per-
minRoleReplicasinner loop and directly compute replicas from the looked-up role.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| roleNames := make(map[string]workloadv1alpha1.Role) | ||
| for _, roleElement := range ms.Spec.Template.Roles { | ||
| roleNames[roleElement.Name] = roleElement | ||
| } |
| for roleName, minReplicas := range minRoleReplicas { | ||
| // Check if the role exists | ||
| if !roleNames[roleName] { | ||
| role, ok := roleMap[roleName] |
| roleNames := make(map[string]bool) | ||
| for _, role := range ms.Spec.Template.Roles { | ||
| roleNames[role.Name] = true | ||
| roleNames := make(map[string]workloadv1alpha1.Role) |
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: