refactor: error checks and nil pointer check (#5964)

* refactor: error checks and nil pointer check * refactor: import sorting order * refactor: lint update * refactor: updating SetCondition to return early if condition is nil
SeldonIO · Oct 9, 2024 · 9b7f7d4 · 9b7f7d4
1 parent 864ad29
commit 9b7f7d4
Show file tree

Hide file tree

Showing 10 changed files with 141 additions and 16 deletions.
diff --git a/operator/apis/mlops/v1alpha1/experiment_types.go b/operator/apis/mlops/v1alpha1/experiment_types.go
@@ -154,7 +154,7 @@ func (es *ExperimentStatus) IsConditionReady(t apis.ConditionType) bool {
 func (es *ExperimentStatus) SetCondition(conditionType apis.ConditionType, condition *apis.Condition) {
 	switch {
 	case condition == nil:
-		experimentConditionSet.Manage(es).MarkUnknown(conditionType, "", "")
+		return
 	case condition.Status == v1.ConditionUnknown:
 		experimentConditionSet.Manage(es).MarkUnknown(conditionType, condition.Reason, condition.Message)
 	case condition.Status == v1.ConditionTrue:

diff --git a/operator/apis/mlops/v1alpha1/experiment_types_test.go b/operator/apis/mlops/v1alpha1/experiment_types_test.go
@@ -17,6 +17,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"knative.dev/pkg/apis"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
+	"knative.dev/pkg/ptr"
 
 	"github.com/seldonio/seldon-core/apis/go/v2/mlops/scheduler"
 )
@@ -210,3 +211,71 @@ func TestExperimentStatusPrintColumns(t *testing.T) {
 		})
 	}
 }
+
+func TestExperimentStatusSetCondition(t *testing.T) {
+	type args struct {
+		conditionType apis.ConditionType
+		condition     *apis.Condition
+	}
+	tests := []struct {
+		name string
+		args args
+		want *v1.ConditionStatus
+	}{
+		{
+			name: "should not panic if condition is nil",
+			args: args{
+				conditionType: ExperimentReady,
+				condition:     nil,
+			},
+			want: nil,
+		},
+		{
+			name: "ConditionUnknown",
+			args: args{
+				conditionType: ExperimentReady,
+				condition: &apis.Condition{
+					Status: "Unknown",
+				},
+			},
+			want: (*v1.ConditionStatus)(ptr.String("Unknown")),
+		},
+		{
+			name: "ConditionTrue",
+			args: args{
+				conditionType: ExperimentReady,
+				condition: &apis.Condition{
+					Status: "True",
+				},
+			},
+			want: (*v1.ConditionStatus)(ptr.String("True")),
+		},
+		{
+			name: "ConditionFalse",
+			args: args{
+				conditionType: ExperimentReady,
+				condition: &apis.Condition{
+					Status: "False",
+				},
+			},
+			want: (*v1.ConditionStatus)(ptr.String("False")),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			es := &ExperimentStatus{}
+			es.SetCondition(tt.args.conditionType, tt.args.condition)
+			if tt.want == nil {
+				if es.GetCondition(tt.args.conditionType) != nil {
+					t.Errorf("want %v : got %v", tt.want, es.GetCondition(tt.args.conditionType))
+				}
+			}
+			if tt.want != nil {
+				got := es.GetCondition(tt.args.conditionType).Status
+				if *tt.want != got {
+					t.Errorf("want %v : got %v", *tt.want, got)
+				}
+			}
+		})
+	}
+}
diff --git a/operator/apis/mlops/v1alpha1/pipeline_types.go b/operator/apis/mlops/v1alpha1/pipeline_types.go
@@ -280,7 +280,7 @@ func (ps *PipelineStatus) IsConditionReady(t apis.ConditionType) bool {
 func (ps *PipelineStatus) SetCondition(conditionType apis.ConditionType, condition *apis.Condition) {
 	switch {
 	case condition == nil:
-		pipelineConditionSet.Manage(ps).MarkUnknown(conditionType, "", "")
+		return
 	case condition.Status == v1.ConditionUnknown:
 		pipelineConditionSet.Manage(ps).MarkUnknown(conditionType, condition.Reason, condition.Message)
 	case condition.Status == v1.ConditionTrue:

diff --git a/operator/apis/mlops/v1alpha1/seldonruntime_types.go b/operator/apis/mlops/v1alpha1/seldonruntime_types.go
@@ -116,7 +116,7 @@ func (ss *SeldonRuntimeStatus) IsConditionReady(t apis.ConditionType) bool {
 func (ss *SeldonRuntimeStatus) SetCondition(condition *apis.Condition) {
 	switch {
 	case condition == nil:
-		seldonRuntimeConditionSet.Manage(ss).MarkUnknown(condition.Type, "", "")
+		return
 	case condition.Status == v1.ConditionUnknown:
 		seldonRuntimeConditionSet.Manage(ss).MarkUnknown(condition.Type, condition.Reason, condition.Message)
 	case condition.Status == v1.ConditionTrue:

diff --git a/operator/apis/mlops/v1alpha1/server_types.go b/operator/apis/mlops/v1alpha1/server_types.go
@@ -138,7 +138,7 @@ func (ss *ServerStatus) IsConditionReady(t apis.ConditionType) bool {
 func (ss *ServerStatus) SetCondition(condition *apis.Condition) {
 	switch {
 	case condition == nil:
-		serverConditionSet.Manage(ss).MarkUnknown(condition.Type, "", "")
+		return
 	case condition.Status == v1.ConditionUnknown:
 		serverConditionSet.Manage(ss).MarkUnknown(condition.Type, condition.Reason, condition.Message)
 	case condition.Status == v1.ConditionTrue:

diff --git a/operator/apis/mlops/v1alpha1/server_types_test.go b/operator/apis/mlops/v1alpha1/server_types_test.go
@@ -15,6 +15,9 @@ import (
 
 	. "github.com/onsi/gomega"
 	"github.com/tidwall/gjson"
+	v1 "k8s.io/api/core/v1"
+	"knative.dev/pkg/apis"
+	"knative.dev/pkg/ptr"
 )
 
 /* WARNING: Read this first if test below fails (either at compile-time or while running the
@@ -71,3 +74,66 @@ func TestServerStatusPrintColumns(t *testing.T) {
 		})
 	}
 }
+
+func TestServerStatusSetCondition(t *testing.T) {
+	type args struct {
+		condition *apis.Condition
+	}
+	tests := []struct {
+		name string
+		args args
+		want *v1.ConditionStatus
+	}{
+		{
+			name: "should not panic if condition is nil",
+			args: args{
+				condition: nil,
+			},
+			want: nil,
+		},
+		{
+			name: "ConditionUnknown",
+			args: args{
+				condition: &apis.Condition{
+					Status: "Unknown",
+				},
+			},
+			want: (*v1.ConditionStatus)(ptr.String("Unknown")),
+		},
+		{
+			name: "ConditionTrue",
+			args: args{
+				condition: &apis.Condition{
+					Status: "True",
+				},
+			},
+			want: (*v1.ConditionStatus)(ptr.String("True")),
+		},
+		{
+			name: "ConditionFalse",
+			args: args{
+				condition: &apis.Condition{
+					Status: "False",
+				},
+			},
+			want: (*v1.ConditionStatus)(ptr.String("False")),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ss := &ServerStatus{}
+			ss.SetCondition(tt.args.condition)
+			if tt.want == nil {
+				if ss.GetCondition("") != nil {
+					t.Errorf("want %v : got %v", tt.want, ss.GetCondition(""))
+				}
+			}
+			if tt.want != nil {
+				got := ss.GetCondition("").Status
+				if *tt.want != got {
+					t.Errorf("want %v : got %v", *tt.want, got)
+				}
+			}
+		})
+	}
+}
diff --git a/operator/pkg/cli/scheduler.go b/operator/pkg/cli/scheduler.go
@@ -595,9 +595,6 @@ func (sc *SchedulerClient) LoadPipeline(data []byte) (*scheduler.LoadPipelineRes
 		return nil, err
 	}
 	schPipeline := pipeline.AsSchedulerPipeline()
-	if err != nil {
-		return nil, err
-	}
 	req := &scheduler.LoadPipelineRequest{Pipeline: schPipeline}
 	if sc.verbose {
 		printProto(req)

diff --git a/scheduler/pkg/kafka/gateway/worker_test.go b/scheduler/pkg/kafka/gateway/worker_test.go
@@ -154,11 +154,7 @@ type mockGRPCMLServer struct {
 }
 
 func (m *mockGRPCMLServer) setup() error {
-	var err error
 	m.listener = bufconn.Listen(bufSize)
-	if err != nil {
-		return err
-	}
 	opts := []grpc.ServerOption{}
 	m.server = grpc.NewServer(opts...)
 	v2.RegisterGRPCInferenceServiceServer(m.server, m)

diff --git a/scheduler/pkg/kafka/pipeline/status/model_rest.go b/scheduler/pkg/kafka/pipeline/status/model_rest.go
@@ -37,9 +37,6 @@ func NewModelRestStatusCaller(logger logrus.FieldLogger, envoyHost string, envoy
 		return nil, err
 	}
 	httpClient := util.GetHttpClientFromTLSOptions(tlsOptions)
-	if err != nil {
-		return nil, err
-	}
 	return &ModelRestCaller{
 		envoyHost:  envoyHost,
 		envoyPort:  envoyPort,

diff --git a/scheduler/pkg/proxy/chainer.go b/scheduler/pkg/proxy/chainer.go
@@ -72,12 +72,12 @@ func (pc *ProxyChainer) SubscribePipelineUpdates(
 				Version:  1,
 				Uid:      "1234",
 				Updates: []*chainer.PipelineStepUpdate{
-					&chainer.PipelineStepUpdate{
+					{
 						Sources:     []*chainer.PipelineTopic{{PipelineName: "some-pipeline", TopicName: chainerInputTopic1}},
 						Sink:        &chainer.PipelineTopic{PipelineName: "some-pipeline", TopicName: chainerOutputTopic1},
 						InputJoinTy: chainer.PipelineStepUpdate_Inner,
 					},
-					&chainer.PipelineStepUpdate{
+					{
 						Sources:     []*chainer.PipelineTopic{{PipelineName: "some-pipeline", TopicName: chainerInputTopic2}},
 						Sink:        &chainer.PipelineTopic{PipelineName: "some-pipeline", TopicName: chainerOutputTopic2},
 						InputJoinTy: chainer.PipelineStepUpdate_Inner,