From cb6b91c5dc14bbd467dfa6e231a4981880394509 Mon Sep 17 00:00:00 2001
From: yxxhero <11087727+yxxhero@users.noreply.github.com>
Date: Wed, 13 Dec 2023 15:04:20 +0800
Subject: [PATCH] fix: ConditionEnabled panic issue (#1221)

---
 pkg/state/state.go      | 30 ++++++++++++++++++++++--------
 pkg/state/state_test.go | 25 +++++++++++++++----------
 2 files changed, 37 insertions(+), 18 deletions(-)

diff --git a/pkg/state/state.go b/pkg/state/state.go
index d686e27c..b1765b46 100644
--- a/pkg/state/state.go
+++ b/pkg/state/state.go
@@ -2244,8 +2244,16 @@ func markExcludedReleases(releases []ReleaseSpec, selectors []string, commonLabe
 	return filteredReleases, nil
 }
 
+// ConditionEnabled checks if a release condition is enabled based on the provided values.
+// It takes a ReleaseSpec and a map of values as input.
+// If the condition is not specified, it returns true.
+// If the condition is specified but not in the form 'foo.enabled', it returns an error.
+// If the condition is specified and the corresponding value is found in the values map,
+// it checks if the 'enabled' field is set to true. If so, it returns true.
+// Otherwise, it returns false.
+// If the condition is specified but the corresponding value is not found in the values map,
+// it returns an error.
 func ConditionEnabled(r ReleaseSpec, values map[string]any) (bool, error) {
-	var conditionMatch bool
 	if len(r.Condition) == 0 {
 		return true, nil
 	}
@@ -2255,16 +2263,22 @@ func ConditionEnabled(r ReleaseSpec, values map[string]any) (bool, error) {
 	}
 	if v, ok := values[conditionSplit[0]]; ok {
 		if v == nil {
-			panic(fmt.Sprintf("environment values field '%s' is nil", conditionSplit[0]))
+			return false, fmt.Errorf("environment values field '%s' is nil", conditionSplit[0])
 		}
-		if v.(map[string]any)["enabled"] == true {
-			conditionMatch = true
+		vm, ok := v.(map[string]any)
+		if !ok {
+			return false, fmt.Errorf("environment values field '%s' is not a map", conditionSplit[0])
 		}
-	} else {
-		panic(fmt.Sprintf("environment values does not contain field '%s'", conditionSplit[0]))
+		vv, ok := vm["enabled"]
+		if !ok {
+			return false, nil
+		}
+		if vv == true {
+			return true, nil
+		}
+		return false, nil
 	}
-
-	return conditionMatch, nil
+	return false, fmt.Errorf("environment values does not contain field '%s'", conditionSplit[0])
 }
 
 func unmarkNeedsAndTransitives(filteredReleases []Release, allReleases []ReleaseSpec) {
diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go
index 1c999b92..b504e464 100644
--- a/pkg/state/state_test.go
+++ b/pkg/state/state_test.go
@@ -2429,7 +2429,6 @@ func TestConditionEnabled(t *testing.T) {
 		values    map[string]any
 		want      bool
 		wantErr   bool
-		wantPanic bool
 	}{
 		{
 			name:      "enabled",
@@ -2451,6 +2450,17 @@ func TestConditionEnabled(t *testing.T) {
 			},
 			want: false,
 		},
+		{
+			name:      "typo in condition",
+			condition: "fooo.enabled",
+			values: map[string]any{
+				"foo": map[string]any{
+					"enabled": true,
+				},
+			},
+			want:    false,
+			wantErr: true,
+		},
 		{
 			name:      "missing enabled",
 			condition: "foo.enabled",
@@ -2467,13 +2477,15 @@ func TestConditionEnabled(t *testing.T) {
 			values: map[string]any{
 				"foo": nil,
 			},
-			wantPanic: true,
+			want:    false,
+			wantErr: true,
 		},
 		{
 			name:      "foo missing",
 			condition: "foo.enabled",
 			values:    map[string]any{},
-			wantPanic: true,
+			want:      false,
+			wantErr:   true,
 		},
 		{
 			name:      "wrong suffix",
@@ -2502,13 +2514,6 @@ func TestConditionEnabled(t *testing.T) {
 	for i := range tests {
 		tt := tests[i]
 		t.Run(tt.name, func(t *testing.T) {
-			if tt.wantPanic {
-				defer func() {
-					if r := recover(); r == nil {
-						t.Errorf("ConditionEnabled() for %s expected panic", tt.name)
-					}
-				}()
-			}
 			res, err := ConditionEnabled(ReleaseSpec{Condition: tt.condition}, tt.values)
 			if tt.wantErr {
 				if err == nil {