Add more utility functions for job and status #14
Conversation
|
Hi @jian-he. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
For the UT, since it requires other code to be available to write a full UT, can it be added later ? |
|
Can we add some unit tests? |
|
sure, let me do it |
job_controller/status.go
Outdated
| // labels for pods and servers. | ||
| ReplicaTypeLabel = "replica-type" | ||
| ReplicaIndexLabel = "replica-index" | ||
| labelGroupName = "group-name" |
There was a problem hiding this comment.
We define the label as private but it is not used in the package, should we define it as a global var or just delete it?
There was a problem hiding this comment.
This is used in the newly added TestJobController
util/k8sutil/k8sutil.go
Outdated
| @@ -121,3 +122,19 @@ func FilterPodCount(pods []*v1.Pod, phase v1.PodPhase) int32 { | |||
| } | |||
| return result | |||
| } | |||
|
|
|||
| func GetTotalReplicas(replicas map[commonv1.ReplicaType]*commonv1.ReplicaSpec) int32 { | |||
| tfjobReplicas := int32(0) | |||
There was a problem hiding this comment.
Remove mentions of "tfjob" in this function
| @@ -72,7 +79,7 @@ type ControllerInterface interface { | |||
| CreateService(job interface{}, service *v1.Service) error | |||
|
|
|||
| // DeleteService deletes the service | |||
| DeleteService(job interface{}, service *v1.Service) error | |||
| DeleteService(job interface{}, name string, namespace string) error | |||
There was a problem hiding this comment.
Can we make function signature consistent with CreateService?
There was a problem hiding this comment.
Personally, I think we do not need to compose a service object to delete it.
There was a problem hiding this comment.
Is service argument meant to have the entire Service Object? What is the job argument for?
There was a problem hiding this comment.
the job argument is for the purpose of logging with job contexts such as its name
e.g the r.Recorder.Eventf takes in the job as the argument
|
Pushed a new change. |
|
/lgtm |
| corev1 "k8s.io/api/core/v1" | ||
| ) | ||
|
|
||
| func TestIsSucceeded(t *testing.T) { |
|
|
||
| func TestUpdateJobReplicaStatuses(t *testing.T) { | ||
| jobStatus := v1.JobStatus{} | ||
| initializeReplicaStatuses(&jobStatus, "worker") |
There was a problem hiding this comment.
Add asserts here for the initialized statuses
| } | ||
|
|
||
| // filterOutCondition returns a new slice of job conditions without conditions with the provided type. | ||
| func filterOutCondition(conditions []common.JobCondition, condType common.JobConditionType) []common.JobCondition { |
| // setCondition updates the job to include the provided condition. | ||
| // If the condition that we are about to add already exists | ||
| // and has the same status and reason then we are not going to update. | ||
| func setCondition(status *common.JobStatus, condition common.JobCondition) { |
There was a problem hiding this comment.
Should have a unit test for this.
There was a problem hiding this comment.
This is called by updateJobConditions and already covered by TestUpdateJobConditions
|
@richardsliu could you check the new commit ? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: richardsliu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add more utility functions for job and status.
This echos what's already available in TF operator.
@terrytangyuan
@richardsliu
@johnugeorge
@gaocegege
This change is