[Refactor] Optimize the logic and readability of the waitDependsOnTas… #4191
Conversation
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
volcano-sh-bot
added
the
size/S
|
Please complete your description |
|
ok |
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/controllers/job/job_controller_actions.go:581
- [nitpick] Consider using a consistent variable name (e.g., 'dependsOnTask') instead of 'taskName' to maintain consistency with previous naming and improve readability.
for _, taskName := range dependsOn.Name {
| @@ -567,7 +567,8 @@ func (cc *jobcontroller) waitDependsOnTaskMeetCondition(taskIndex int, job *batc | |||
| return true | |||
| } | |||
| dependsOn := *job.Spec.Tasks[taskIndex].DependsOn | |||
| if len(dependsOn.Name) > 1 && dependsOn.Iteration == batch.IterationAny { | |||
| switch dependsOn.Iteration { | |||
There was a problem hiding this comment.
Ensure that the default branch's logic for non-IterationAny cases matches the intended behavior of requiring all dependency tasks to be ready. Verify that this change preserves the original business logic from the previous if-statement condition.
Copilot uses AI. Check for mistakes.
|
Should add ut when refactoring. |
volcano-sh-bot
added
size/L
ok.. |
|
Hi, please squash to one commit. |
ok |
| if cc.isDependsOnPodsReady(task, job) { | ||
| switch dependsOn.Iteration { | ||
| case batch.IterationAny: | ||
| // any ready to create task, return truechi |
| @@ -863,3 +864,126 @@ func TestRecordPodGroupEvent(t *testing.T) { | |||
| } | |||
|
|
|||
| } | |||
|
|
|||
| func TestWaitDependsOnTaskMeetCondition(t *testing.T) { | |||
There was a problem hiding this comment.
UT is not correct, please re-write it, the expected value should be a bool type.
sample
func TestWaitDependsOnTaskMeetCondition(t *testing.T) {
namespace := "test"
testcases := []struct {
Name string
Job *v1alpha1.Job
TaskIndex int
JobInfo *apis.JobInfo
Pods map[string]map[string]*v1.Pod
ExpectResult bool
}
....
There was a problem hiding this comment.
I'll make the change right away
…kMeetCondition function Signed-off-by: feyounger <[email protected]> [Refactor] Optimize the logic and readability of the waitDependsOnTaskMeetCondition function ut Signed-off-by: feyounger <[email protected]> [Refactor] Cover case checkControllers ut Signed-off-by: feyounger <[email protected]> [Refactor] update TestWaitDependsOnTaskMeetCondition ExpectVal type Signed-off-by: feyounger <[email protected]>
…kMeetCondition function
What type of PR is this?
Related with #4190 code
What this PR does / why we need it:
Make the structure fresher
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?