Skip to content

Commit 9e275d9

Browse files
asn1809asn1809
committed
Correcting lint issues and adding unit tests
Signed-off-by: Annaraya Narasagond <[email protected]> Signed-off-by: Annaraya Narasagond <[email protected]> Annaraya Narasagond <[email protected]>
1 parent dc44123 commit 9e275d9

File tree

3 files changed

+252
-48
lines changed

3 files changed

+252
-48
lines changed

‎internal/controller/util/json_util.go

Lines changed: 40 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func EvaluateCheckHook(client client.Client, hook *kubeobjects.HookSpec, log log
3939
return false, err
4040
}
4141

42-
return evaluateCheckHookExp(hook.Chk.Condition, resource)
42+
return EvaluateCheckHookExp(hook.Chk.Condition, resource)
4343
case "deployment":
4444
// handle deployment type
4545
resource := &appsv1.Deployment{}
@@ -49,7 +49,7 @@ func EvaluateCheckHook(client client.Client, hook *kubeobjects.HookSpec, log log
4949
return false, err
5050
}
5151

52-
return evaluateCheckHookExp(hook.Chk.Condition, resource)
52+
return EvaluateCheckHookExp(hook.Chk.Condition, resource)
5353
case "statefulset":
5454
// handle statefulset type
5555
resource := &appsv1.StatefulSet{}
@@ -59,7 +59,7 @@ func EvaluateCheckHook(client client.Client, hook *kubeobjects.HookSpec, log log
5959
return false, err
6060
}
6161

62-
return evaluateCheckHookExp(hook.Chk.Condition, resource)
62+
return EvaluateCheckHookExp(hook.Chk.Condition, resource)
6363
}
6464

6565
return false, nil
@@ -104,7 +104,7 @@ func getTimeoutValue(hook *kubeobjects.HookSpec) int {
104104
return defaultTimeoutValue
105105
}
106106

107-
func evaluateCheckHookExp(booleanExpression string, jsonData interface{}) (bool, error) {
107+
func EvaluateCheckHookExp(booleanExpression string, jsonData interface{}) (bool, error) {
108108
op, jsonPaths, err := parseBooleanExpression(booleanExpression)
109109
if err != nil {
110110
return false, fmt.Errorf("failed to parse boolean expression: %w", err)
@@ -197,6 +197,36 @@ func compareBool(a, b bool, operator string) (bool, error) {
197197
}
198198
}
199199

200+
func compareString(a, b, operator string) (bool, error) {
201+
switch operator {
202+
case "==":
203+
return a == b, nil
204+
case "!=":
205+
return a != b, nil
206+
default:
207+
return false, fmt.Errorf("unknown operator: %s", operator)
208+
}
209+
}
210+
211+
func compareFloat(a, b float64, operator string) (bool, error) {
212+
switch operator {
213+
case "==":
214+
return a == b, nil
215+
case "!=":
216+
return a != b, nil
217+
case "<":
218+
return a < b, nil
219+
case ">":
220+
return a > b, nil
221+
case "<=":
222+
return a <= b, nil
223+
case ">=":
224+
return a >= b, nil
225+
default:
226+
return false, fmt.Errorf("unknown operator: %s", operator)
227+
}
228+
}
229+
200230
func compareValues(val1, val2 interface{}, operator string) (bool, error) {
201231
switch v1 := val1.(type) {
202232
case float64:
@@ -205,44 +235,21 @@ func compareValues(val1, val2 interface{}, operator string) (bool, error) {
205235
return false, fmt.Errorf("mismatched types")
206236
}
207237

208-
switch operator {
209-
case "==":
210-
return v1 == v2, nil
211-
case "!=":
212-
return v1 != v2, nil
213-
case "<":
214-
return v1 < v2, nil
215-
case ">":
216-
return v1 > v2, nil
217-
case "<=":
218-
return v1 <= v2, nil
219-
case ">=":
220-
return v1 >= v2, nil
221-
}
238+
return compareFloat(v1, v2, operator)
222239
case string:
223240
v2, ok := val2.(string)
224241
if !ok {
225242
return false, fmt.Errorf("mismatched types")
226243
}
227244

228-
switch operator {
229-
case "==":
230-
return v1 == v2, nil
231-
case "!=":
232-
return v1 != v2, nil
233-
}
245+
return compareString(v1, v2, operator)
234246
case bool:
235247
v2, ok := val2.(bool)
236248
if !ok {
237249
return false, fmt.Errorf("mismatched types")
238250
}
239251

240-
switch operator {
241-
case "==":
242-
return v1 == v2, nil
243-
case "!=":
244-
return v1 != v2, nil
245-
}
252+
return compareBool(v1, v2, operator)
246253
}
247254

248255
return false, fmt.Errorf("unsupported type or operator")
@@ -281,16 +288,16 @@ func parseBooleanExpression(booleanExpression string) (op string, jsonPaths []st
281288
jsonPaths = trimLeadingTrailingWhiteSpace(exprs)
282289

283290
if len(exprs) == 2 &&
284-
isValidJSONPathExpression(jsonPaths[0]) &&
285-
isValidJSONPathExpression(jsonPaths[1]) {
291+
IsValidJSONPathExpression(jsonPaths[0]) &&
292+
IsValidJSONPathExpression(jsonPaths[1]) {
286293
return op, jsonPaths, nil
287294
}
288295
}
289296

290297
return "", []string{}, fmt.Errorf("unable to parse boolean expression %v", booleanExpression)
291298
}
292299

293-
func isValidJSONPathExpression(expr string) bool {
300+
func IsValidJSONPathExpression(expr string) bool {
294301
jp := jsonpath.New("validator").AllowMissingKeys(true)
295302

296303
err := jp.Parse(expr)

‎internal/controller/util/json_util_test.go

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
package util_test
2+
3+
import (
4+
"encoding/json"
5+
"strconv"
6+
"testing"
7+
8+
"github.com/ramendr/ramen/internal/controller/util"
9+
)
10+
11+
type testCases struct {
12+
jsonPathExprs string
13+
result bool
14+
}
15+
16+
var jsonText1 = []byte(`{
17+
"kind": "Deployment",
18+
"spec": {
19+
"progressDeadlineSeconds": 600,
20+
"replicas": 1,
21+
"revisionHistoryLimit": 10
22+
},
23+
"status": {
24+
"replicas": 1,
25+
"conditions": [
26+
{
27+
"status": "True",
28+
"type": "Progressing"
29+
},
30+
{
31+
"status": "True",
32+
"type": "Available"
33+
}
34+
]
35+
}
36+
}`)
37+
38+
var testCasesData = []testCases{
39+
{
40+
jsonPathExprs: "{$.status.conditions[0].status} == True",
41+
result: true,
42+
},
43+
{
44+
jsonPathExprs: "{$.spec.replicas} == 1",
45+
result: true,
46+
},
47+
{
48+
jsonPathExprs: "{$.status.conditions[0].status} == {True}",
49+
result: false,
50+
},
51+
}
52+
53+
func TestXYZ(t *testing.T) {
54+
for i, tt := range testCasesData {
55+
t.Run(strconv.Itoa(i), func(t *testing.T) {
56+
var jsonData map[string]interface{}
57+
err := json.Unmarshal(jsonText1, &jsonData)
58+
if err != nil {
59+
t.Error(err)
60+
}
61+
_, err = util.EvaluateCheckHookExp(tt.jsonPathExprs, jsonData)
62+
if (err == nil) != tt.result {
63+
t.Errorf("EvaluateCheckHook() = %v, want %v", err, tt.result)
64+
}
65+
})
66+
}
67+
}
68+
69+
func Test_isValidJsonPathExpression(t *testing.T) {
70+
type args struct {
71+
expr string
72+
}
73+
tests := []struct {
74+
name string
75+
args args
76+
want bool
77+
}{
78+
{
79+
name: "Simple expression",
80+
args: args{
81+
expr: "$.spec.replicas",
82+
},
83+
want: true,
84+
},
85+
{
86+
name: "no $ at the start",
87+
args: args{
88+
expr: "{.spec.replicas}",
89+
},
90+
want: true,
91+
},
92+
{
93+
name: "element in array",
94+
args: args{
95+
expr: "{$.status.conditions[0].status}",
96+
},
97+
want: true,
98+
},
99+
{
100+
name: "spec 1",
101+
args: args{
102+
expr: "{$.status.readyReplicas}",
103+
},
104+
want: true,
105+
},
106+
{
107+
name: "spec 2",
108+
args: args{
109+
expr: "{$.status.containerStatuses[0].ready}",
110+
},
111+
want: true,
112+
},
113+
{
114+
name: "spec 3a",
115+
args: args{
116+
expr: "{True}",
117+
},
118+
want: false,
119+
},
120+
{
121+
name: "spec 3b",
122+
args: args{
123+
expr: "{False}",
124+
},
125+
want: false,
126+
},
127+
{
128+
name: "spec 3c",
129+
args: args{
130+
expr: "{true}",
131+
},
132+
want: true,
133+
},
134+
{
135+
name: "spec 3d",
136+
args: args{
137+
expr: "{false}",
138+
},
139+
want: true,
140+
},
141+
{
142+
name: "Spec 4",
143+
args: args{
144+
expr: "{$.spec.replicas}",
145+
},
146+
want: true,
147+
},
148+
{
149+
name: "expression with == operator",
150+
args: args{
151+
expr: "$.store.book[?(@.price > 10)].title==$.store.book[0].title",
152+
},
153+
want: true,
154+
},
155+
{
156+
name: "expression with > operator",
157+
args: args{
158+
expr: "$.store.book[?(@.author CONTAINS 'Smith')].price>20",
159+
},
160+
want: true,
161+
},
162+
{
163+
name: "expression with >= operator",
164+
args: args{
165+
expr: "$.user.age>=$.minimum.age",
166+
},
167+
want: true,
168+
},
169+
{
170+
name: "expression with < operator",
171+
args: args{
172+
expr: "$.user.age<$.maximum.age",
173+
},
174+
want: true,
175+
},
176+
{
177+
name: "expression with <= operator",
178+
args: args{
179+
expr: "$.user.age<=$.maximum.age",
180+
},
181+
want: true,
182+
},
183+
{
184+
name: "expression with != operator",
185+
args: args{
186+
expr: "$.user.age!=$.maximum.age",
187+
},
188+
want: true,
189+
},
190+
}
191+
192+
for _, tt := range tests {
193+
t.Run(tt.name, func(t *testing.T) {
194+
if got := util.IsValidJSONPathExpression(tt.args.expr); got != tt.want {
195+
t.Errorf("isValidJsonPathExpression() = %v, want %v", got, tt.want)
196+
}
197+
})
198+
}
199+
}

‎internal/controller/vrg_kubeobjects.go

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -259,14 +259,12 @@ func (v *VRGInstance) kubeObjectsCaptureStartOrResume(
259259
log1.Info("Check hook executed successfully", "check hook is ", hookName, " result is ", hookResult)
260260
}
261261

262-
if !hookResult {
263-
if shouldHookBeFailedOnError(&cg.Hook) {
264-
// update error state
265-
break
266-
}
262+
if !hookResult && shouldHookBeFailedOnError(&cg.Hook) {
267263
// update error state
268-
continue
264+
break
269265
}
266+
// update error state
267+
continue
270268
}
271269
} else {
272270
requestsCompletedCount += v.kubeObjectsGroupCapture(
@@ -632,16 +630,16 @@ func (v *VRGInstance) kubeObjectsRecoveryStartOrResume(
632630
log1 := log.WithValues("group", groupNumber, "name", rg.BackupName)
633631

634632
if rg.IsHook {
635-
hookResult, err := util.EvaluateCheckHook(v.reconciler.Client, &rg.Hook, log1)
636-
if err != nil {
637-
log.Error(err, "error occurred during check hook ")
638-
} else {
639-
hookName := rg.Hook.Name + "/" + rg.Hook.Chk.Name
640-
log1.Info("Check hook executed successfully", "check hook is ", hookName, " result is ", hookResult)
641-
}
633+
if rg.Hook.Type == "check" {
634+
hookResult, err := util.EvaluateCheckHook(v.reconciler.Client, &rg.Hook, log1)
635+
if err != nil {
636+
log.Error(err, "error occurred during check hook ")
637+
} else {
638+
hookName := rg.Hook.Name + "/" + rg.Hook.Chk.Name
639+
log1.Info("Check hook executed successfully", "check hook is ", hookName, " result is ", hookResult)
640+
}
642641

643-
if !hookResult {
644-
if shouldHookBeFailedOnError(&rg.Hook) {
642+
if !hookResult && shouldHookBeFailedOnError(&rg.Hook) {
645643
// update error state
646644
break
647645
}

0 commit comments

Comments
 (0)