Skip to content

Commit 16b80c7

Browse files
aali309aali309
committed
more formatting and fix test errors
Signed-off-by: Atif Ali <[email protected]>
1 parent 5d677d5 commit 16b80c7

File tree

2 files changed

+174
-42
lines changed

2 files changed

+174
-42
lines changed

‎pkg/sync/sync_context.go

+70-21
Original file line numberDiff line numberDiff line change
@@ -973,20 +973,23 @@ func formatValue(v interface{}) string {
973973

974974
// Special case for volumeClaimTemplates
975975
if templates, ok := v.([]interface{}); ok {
976+
// For a single volumeClaimTemplate field change
977+
if len(templates) == 1 {
978+
if template, ok := templates[0].(map[string]interface{}); ok {
979+
if storage := getTemplateStorage(template); storage != "" {
980+
return fmt.Sprintf("%q", storage)
981+
}
982+
}
983+
}
984+
// For multiple templates or other array types format
976985
var names []string
977986
for _, t := range templates {
978987
if template, ok := t.(map[string]interface{}); ok {
979988
if metadata, ok := template["metadata"].(map[string]interface{}); ok {
980989
if name, ok := metadata["name"].(string); ok {
981-
if spec, ok := template["spec"].(map[string]interface{}); ok {
982-
if resources, ok := spec["resources"].(map[string]interface{}); ok {
983-
if requests, ok := resources["requests"].(map[string]interface{}); ok {
984-
if storage, ok := requests["storage"].(string); ok {
985-
names = append(names, fmt.Sprintf("%s(%s)", name, storage))
986-
continue
987-
}
988-
}
989-
}
990+
if storage := getTemplateStorage(template); storage != "" {
991+
names = append(names, fmt.Sprintf("%s(%s)", name, storage))
992+
continue
990993
}
991994
names = append(names, name)
992995
}
@@ -1007,16 +1010,41 @@ func formatValue(v interface{}) string {
10071010
return fmt.Sprintf("{%s}", strings.Join(labels, ", "))
10081011
}
10091012
}
1010-
10111013
// Add quotes for string values
10121014
if str, ok := v.(string); ok {
10131015
return fmt.Sprintf("%q", str)
10141016
}
1015-
10161017
// For other types, use standard formatting
10171018
return fmt.Sprintf("%v", v)
10181019
}
10191020

1021+
// Get storage size from template
1022+
func getTemplateStorage(template map[string]interface{}) string {
1023+
spec, ok := template["spec"].(map[string]interface{})
1024+
if !ok {
1025+
return ""
1026+
}
1027+
resources, ok := spec["resources"].(map[string]interface{})
1028+
if !ok {
1029+
return ""
1030+
}
1031+
requests, ok := resources["requests"].(map[string]interface{})
1032+
if !ok {
1033+
return ""
1034+
}
1035+
storage, ok := requests["storage"].(string)
1036+
if !ok {
1037+
return ""
1038+
}
1039+
return storage
1040+
}
1041+
1042+
// Format field changes for error messages
1043+
func formatFieldChange(field string, currentVal, desiredVal interface{}) string {
1044+
return fmt.Sprintf(" - %s:\n from: %s\n to: %s",
1045+
field, formatValue(currentVal), formatValue(desiredVal))
1046+
}
1047+
10201048
func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.ResultCode, string) {
10211049
dryRunStrategy := cmdutil.DryRunNone
10221050
if dryRun {
@@ -1079,16 +1107,38 @@ func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.R
10791107
for k, desiredVal := range desiredSpec {
10801108
if !mutableFields[k] {
10811109
currentVal, exists := currentSpec[k]
1082-
// Only add to changes if values are actually different
1083-
if !exists || !reflect.DeepEqual(currentVal, desiredVal) {
1084-
// Skip if the values are deeply equal (for complex types like volumeClaimTemplates)
1085-
if exists && reflect.DeepEqual(currentVal, desiredVal) {
1086-
continue
1110+
if !exists {
1111+
changes = append(changes, formatFieldChange(k, nil, desiredVal))
1112+
} else if !reflect.DeepEqual(currentVal, desiredVal) {
1113+
if k == "volumeClaimTemplates" {
1114+
// Handle volumeClaimTemplates specially
1115+
currentTemplates := currentVal.([]interface{})
1116+
desiredTemplates := desiredVal.([]interface{})
1117+
1118+
// If template count differs or we're adding/removing templates,
1119+
// use the standard array format
1120+
if len(currentTemplates) != len(desiredTemplates) {
1121+
changes = append(changes, formatFieldChange(k, currentVal, desiredVal))
1122+
} else {
1123+
// Compare each template
1124+
for i, desired := range desiredTemplates {
1125+
current := currentTemplates[i]
1126+
desiredTemplate := desired.(map[string]interface{})
1127+
currentTemplate := current.(map[string]interface{})
1128+
1129+
name := desiredTemplate["metadata"].(map[string]interface{})["name"].(string)
1130+
desiredStorage := getTemplateStorage(desiredTemplate)
1131+
currentStorage := getTemplateStorage(currentTemplate)
1132+
1133+
if currentStorage != desiredStorage {
1134+
changes = append(changes, fmt.Sprintf(" - volumeClaimTemplates.%s:\n from: %q\n to: %q",
1135+
name, currentStorage, desiredStorage))
1136+
}
1137+
}
1138+
}
1139+
} else {
1140+
changes = append(changes, formatFieldChange(k, currentVal, desiredVal))
10871141
}
1088-
currentStr := formatValue(currentVal)
1089-
desiredStr := formatValue(desiredVal)
1090-
changes = append(changes, fmt.Sprintf(" - %s:\n from: %s\n to: %s",
1091-
k, currentStr, desiredStr))
10921142
}
10931143
}
10941144
}
@@ -1100,7 +1150,6 @@ func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.R
11001150
}
11011151
}
11021152
}
1103-
// For all other errors, return the original error message
11041153
return common.ResultCodeSyncFailed, err.Error()
11051154
}
11061155
if kube.IsCRD(t.targetObj) && !dryRun {

‎pkg/sync/sync_context_test.go

+104-21
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,12 @@ import (
3838
testingutils "github.com/argoproj/gitops-engine/pkg/utils/testing"
3939
)
4040

41+
const (
42+
testAPIVersion = "apps/v1"
43+
testStatefulSet = "test-statefulset"
44+
testNamespace = "default"
45+
)
46+
4147
var standardVerbs = v1.Verbs{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}
4248

4349
func newTestSyncCtx(getResourceFunc *func(ctx context.Context, config *rest.Config, gvk schema.GroupVersionKind, name string, namespace string) (*unstructured.Unstructured, error), opts ...SyncOpt) *syncContext {
@@ -52,7 +58,7 @@ func newTestSyncCtx(getResourceFunc *func(ctx context.Context, config *rest.Conf
5258
},
5359
},
5460
&v1.APIResourceList{
55-
GroupVersion: "apps/v1",
61+
GroupVersion: testAPIVersion,
5662
APIResources: []v1.APIResource{
5763
{Kind: "Deployment", Group: "apps", Version: "v1", Namespaced: true, Verbs: standardVerbs},
5864
},
@@ -2040,6 +2046,21 @@ func TestWaitForCleanUpBeforeNextWave(t *testing.T) {
20402046

20412047
}
20422048

2049+
func templateWithStorage(name, storage string) map[string]interface{} {
2050+
return map[string]interface{}{
2051+
"metadata": map[string]interface{}{
2052+
"name": name,
2053+
},
2054+
"spec": map[string]interface{}{
2055+
"resources": map[string]interface{}{
2056+
"requests": map[string]interface{}{
2057+
"storage": storage,
2058+
},
2059+
},
2060+
},
2061+
}
2062+
}
2063+
20432064
func TestStatefulSetImmutableFieldErrors(t *testing.T) {
20442065
tests := []struct {
20452066
name string
@@ -2097,9 +2118,9 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina
20972118
},
20982119
},
20992120
expectedMessage: `attempting to change immutable fields:
2100-
- volumeClaimTemplates:
2101-
from: [data(1Gi)]
2102-
to: [data(2Gi)]
2121+
- volumeClaimTemplates.data:
2122+
from: "1Gi"
2123+
to: "2Gi"
21032124
21042125
Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden`,
21052126
},
@@ -2181,22 +2202,84 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina
21812202
Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden`,
21822203
},
21832204
{
2184-
name: "multiple immutable field changes",
2205+
name: "multiple volumeClaimTemplate change",
2206+
currentSpec: map[string]interface{}{
2207+
"serviceName": "postgresql-svc",
2208+
"selector": map[string]interface{}{
2209+
"matchLabels": map[string]interface{}{
2210+
"app": "postgresql",
2211+
},
2212+
},
2213+
"volumeClaimTemplates": []interface{}{
2214+
templateWithStorage("static-files", "1Gi"),
2215+
templateWithStorage("dexconfig", "1Gi"),
2216+
templateWithStorage("argocd-dex-server-tls", "1Gi"),
2217+
},
2218+
},
2219+
desiredSpec: map[string]interface{}{
2220+
"serviceName": "postgresql-svc",
2221+
"selector": map[string]interface{}{
2222+
"matchLabels": map[string]interface{}{
2223+
"app": "postgresql",
2224+
},
2225+
},
2226+
"volumeClaimTemplates": []interface{}{
2227+
templateWithStorage("static-files", "2Gi"),
2228+
templateWithStorage("dexconfig", "3Gi"),
2229+
templateWithStorage("argocd-dex-server-tls", "4Gi"),
2230+
},
2231+
},
2232+
expectedMessage: `attempting to change immutable fields:
2233+
- volumeClaimTemplates.argocd-dex-server-tls:
2234+
from: "1Gi"
2235+
to: "4Gi"
2236+
- volumeClaimTemplates.dexconfig:
2237+
from: "1Gi"
2238+
to: "3Gi"
2239+
- volumeClaimTemplates.static-files:
2240+
from: "1Gi"
2241+
to: "2Gi"
2242+
2243+
Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden`,
2244+
},
2245+
{
2246+
name: "multiple field changes",
21852247
currentSpec: map[string]interface{}{
2186-
"serviceName": "old-svc",
2187-
"podManagementPolicy": "OrderedReady",
2248+
"serviceName": "postgresql-svc",
2249+
"selector": map[string]interface{}{
2250+
"matchLabels": map[string]interface{}{
2251+
"app": "postgresql",
2252+
},
2253+
},
2254+
"volumeClaimTemplates": []interface{}{
2255+
templateWithStorage("static-files", "1Gi"),
2256+
templateWithStorage("dexconfig", "1Gi"),
2257+
templateWithStorage("argocd-dex-server-tls", "1Gi"),
2258+
},
21882259
},
21892260
desiredSpec: map[string]interface{}{
2190-
"serviceName": "new-svc",
2191-
"podManagementPolicy": "Parallel",
2261+
"serviceName": "postgresql-svc-new",
2262+
"selector": map[string]interface{}{
2263+
"matchLabels": map[string]interface{}{
2264+
"app": "postgresql-new",
2265+
},
2266+
},
2267+
"volumeClaimTemplates": []interface{}{
2268+
templateWithStorage("static-files", "2Gi"),
2269+
templateWithStorage("dexconfig", "1Gi"),
2270+
templateWithStorage("argocd-dex-server-tls", "1Gi"),
2271+
},
21922272
},
21932273
expectedMessage: `attempting to change immutable fields:
2194-
- podManagementPolicy:
2195-
from: "OrderedReady"
2196-
to: "Parallel"
2274+
- selector:
2275+
from: {app:postgresql}
2276+
to: {app:postgresql-new}
21972277
- serviceName:
2198-
from: "old-svc"
2199-
to: "new-svc"
2278+
from: "postgresql-svc"
2279+
to: "postgresql-svc-new"
2280+
- volumeClaimTemplates.static-files:
2281+
from: "1Gi"
2282+
to: "2Gi"
22002283
22012284
Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden`,
22022285
},
@@ -2206,23 +2289,23 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina
22062289
t.Run(tt.name, func(t *testing.T) {
22072290
current := &unstructured.Unstructured{
22082291
Object: map[string]interface{}{
2209-
"apiVersion": "apps/v1",
2292+
"apiVersion": testAPIVersion,
22102293
"kind": "StatefulSet",
22112294
"metadata": map[string]interface{}{
2212-
"name": "test-statefulset",
2213-
"namespace": "default",
2295+
"name": testStatefulSet,
2296+
"namespace": testNamespace,
22142297
},
22152298
"spec": tt.currentSpec,
22162299
},
22172300
}
22182301

22192302
desired := &unstructured.Unstructured{
22202303
Object: map[string]interface{}{
2221-
"apiVersion": "apps/v1",
2304+
"apiVersion": testAPIVersion,
22222305
"kind": "StatefulSet",
22232306
"metadata": map[string]interface{}{
2224-
"name": "test-statefulset",
2225-
"namespace": "default",
2307+
"name": testStatefulSet,
2308+
"namespace": testNamespace,
22262309
},
22272310
"spec": tt.desiredSpec,
22282311
},
@@ -2238,7 +2321,7 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina
22382321
// Mock the resource operations to return immutable field error
22392322
mockResourceOps := &kubetest.MockResourceOps{
22402323
Commands: map[string]kubetest.KubectlOutput{
2241-
"test-statefulset": {
2324+
testStatefulSet: {
22422325
Err: errors.New("Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden"),
22432326
},
22442327
},

0 commit comments

Comments
 (0)