fix: Remove dup code (#1022)

* fix: Remove dup code Signed-off-by: Ce Gao <[email protected]> * fix: Return if completed Signed-off-by: Ce Gao <[email protected]>
kubeflow · Jun 5, 2019 · 462bd84 · 462bd84
1 parent d0b973b
commit 462bd84
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 44 deletions.
diff --git a/pkg/controller.v1/tensorflow/status.go b/pkg/controller.v1/tensorflow/status.go
@@ -178,17 +178,6 @@ func (tc *TFController) updateTFJobStatus(tfjob *tfv1.TFJob) error {
 
 // updateTFJobConditions updates the conditions of the given tfjob.
 func updateTFJobConditions(tfjob *tfv1.TFJob, conditionType common.JobConditionType, reason, message string) error {
-	// Check if the condition exists in the conditions.
-	// See https://github.com/kubeflow/pytorch-operator/issues/88
-	for i, c := range tfjob.Status.Conditions {
-		// Found the condition, thus no need to update the LastTransitionTime.
-		if c.Type == conditionType && c.Status == v1.ConditionTrue {
-			tfjob.Status.Conditions[i].LastUpdateTime = metav1.Now()
-			tfjob.Status.Conditions[i].Reason = reason
-			tfjob.Status.Conditions[i].Message = message
-			return nil
-		}
-	}
 	condition := newCondition(conditionType, reason, message)
 	setCondition(&tfjob.Status, condition)
 	return nil
@@ -260,24 +249,26 @@ func isFailed(status common.JobStatus) bool {
 // 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) {
-	// Do nothing if TFJobStatus have failed condition
-	if isFailed(*status) {
+	// Do nothing if TFJobStatus is completed.
+	if isFailed(*status) || isSucceeded(*status) {
 		return
 	}
 
 	currentCond := getCondition(*status, condition.Type)
 
-	// Do nothing if condition doesn't change
-	if currentCond != nil && currentCond.Status == condition.Status && currentCond.Reason == condition.Reason {
-		return
-	}
-
-	// Do not update lastTransitionTime if the status of the condition doesn't change.
-	if currentCond != nil && currentCond.Status == condition.Status {
-		condition.LastTransitionTime = currentCond.LastTransitionTime
+	if currentCond != nil {
+		if currentCond.Status == condition.Status &&
+			currentCond.Reason == condition.Reason &&
+			currentCond.Message == condition.Message {
+			// Do nothing if the condition does not change.
+			return
+		} else if currentCond.Status == condition.Status {
+			// Do not update lastTransitionTime if the status of the condition doesn't change.
+			condition.LastTransitionTime = currentCond.LastTransitionTime
+		}
 	}
 
-	// Append the updated condition to the
+	// Append the updated condition to the status.Conditions.
 	newConditions := filterOutCondition(status.Conditions, condition.Type)
 	status.Conditions = append(newConditions, condition)
 }

diff --git a/pkg/controller.v1beta2/tensorflow/status.go b/pkg/controller.v1beta2/tensorflow/status.go
@@ -156,17 +156,6 @@ func (tc *TFController) updateTFJobStatus(tfjob *tfv1beta2.TFJob) error {
 
 // updateTFJobConditions updates the conditions of the given tfjob.
 func updateTFJobConditions(tfjob *tfv1beta2.TFJob, conditionType common.JobConditionType, reason, message string) error {
-	// Check if the condition exists in the conditions.
-	// See https://github.com/kubeflow/pytorch-operator/issues/88
-	for i, c := range tfjob.Status.Conditions {
-		// Found the condition, thus no need to update the LastTransitionTime.
-		if c.Type == conditionType && c.Status == v1.ConditionTrue {
-			tfjob.Status.Conditions[i].LastUpdateTime = metav1.Now()
-			tfjob.Status.Conditions[i].Reason = reason
-			tfjob.Status.Conditions[i].Message = message
-			return nil
-		}
-	}
 	condition := newCondition(conditionType, reason, message)
 	setCondition(&tfjob.Status, condition)
 	return nil
@@ -238,24 +227,26 @@ func isFailed(status common.JobStatus) bool {
 // 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) {
-	// Do nothing if TFJobStatus have failed condition
-	if isFailed(*status) {
+	// Do nothing if TFJobStatus is completed.
+	if isFailed(*status) || isSucceeded(*status) {
 		return
 	}
 
 	currentCond := getCondition(*status, condition.Type)
 
-	// Do nothing if condition doesn't change
-	if currentCond != nil && currentCond.Status == condition.Status && currentCond.Reason == condition.Reason {
-		return
-	}
-
-	// Do not update lastTransitionTime if the status of the condition doesn't change.
-	if currentCond != nil && currentCond.Status == condition.Status {
-		condition.LastTransitionTime = currentCond.LastTransitionTime
+	if currentCond != nil {
+		if currentCond.Status == condition.Status &&
+			currentCond.Reason == condition.Reason &&
+			currentCond.Message == condition.Message {
+			// Do nothing if the condition does not change.
+			return
+		} else if currentCond.Status == condition.Status {
+			// Do not update lastTransitionTime if the status of the condition doesn't change.
+			condition.LastTransitionTime = currentCond.LastTransitionTime
+		}
 	}
 
-	// Append the updated condition to the
+	// Append the updated condition to the status.Conditions.
 	newConditions := filterOutCondition(status.Conditions, condition.Type)
 	status.Conditions = append(newConditions, condition)
 }