From 60ab95e6fb0cdd2b6a7e94ac61eee7c37226235b Mon Sep 17 00:00:00 2001 From: David Kwon Date: Tue, 22 Sep 2026 20:25:40 -0400 Subject: [PATCH 1/5] Tolerate additional labels under deployment's spec.template.metadata.labels field Signed-off-by: David Kwon Co-authored-by: Claude Opus 4.6 --- pkg/provision/sync/diff.go | 27 +++- pkg/provision/sync/diff_test.go | 219 ++++++++++++++++++++++++++++++++ pkg/provision/sync/diffopts.go | 1 + 3 files changed, 246 insertions(+), 1 deletion(-) create mode 100644 pkg/provision/sync/diff_test.go diff --git a/pkg/provision/sync/diff.go b/pkg/provision/sync/diff.go index 83e115dea..ace79f0b5 100644 --- a/pkg/provision/sync/diff.go +++ b/pkg/provision/sync/diff.go @@ -41,7 +41,7 @@ var diffFuncs = map[reflect.Type]diffFunc{ reflect.TypeOf(rbacv1.Role{}): allDiffFuncs(metadataDiffFunc, basicDiffFunc(roleDiffOpts)), reflect.TypeOf(rbacv1.RoleBinding{}): allDiffFuncs(metadataDiffFunc, basicDiffFunc(rolebindingDiffOpts)), reflect.TypeOf(corev1.ServiceAccount{}): metadataDiffFunc, - reflect.TypeOf(appsv1.Deployment{}): allDiffFuncs(deploymentDiffFunc, metadataDiffFunc, basicDiffFunc(deploymentDiffOpts)), + reflect.TypeOf(appsv1.Deployment{}): allDiffFuncs(deploymentDiffFunc, metadataDiffFunc, podTemplateMetadataDiffFunc, basicDiffFunc(deploymentDiffOpts)), reflect.TypeOf(corev1.Pod{}): allDiffFuncs(podDiffFunc, metadataDiffFunc), reflect.TypeOf(corev1.ConfigMap{}): allDiffFuncs(metadataDiffFunc, basicDiffFunc(configmapDiffOpts)), reflect.TypeOf(corev1.Secret{}): allDiffFuncs(metadataDiffFunc, basicDiffFunc(secretDiffOpts)), @@ -83,6 +83,31 @@ func metadataDiffFunc(spec, cluster crclient.Object) (delete, update bool) { return false, false } +// podTemplateMetadataDiffFunc requires a Deployment to be updated if any label or annotation present in the spec +// deployment's pod template is not present in the cluster deployment's pod template. Like metadataDiffFunc, it only +// checks the spec-to-cluster direction so that externally-added labels on the pod template do not trigger an update. +func podTemplateMetadataDiffFunc(spec, cluster crclient.Object) (delete, update bool) { + specDeploy, ok := spec.(*appsv1.Deployment) + if !ok { + return false, false + } + clusterDeploy := cluster.(*appsv1.Deployment) + + clusterLabels := clusterDeploy.Spec.Template.Labels + for k, v := range specDeploy.Spec.Template.Labels { + if clusterLabels[k] != v { + return false, true + } + } + clusterAnnotations := clusterDeploy.Spec.Template.Annotations + for k, v := range specDeploy.Spec.Template.Annotations { + if clusterAnnotations[k] != v { + return false, true + } + } + return false, false +} + // allDiffFuncs represents an 'and' condition across specified diffFuncs. Functions are checked in provided order, // returning the result of the first function to require an update/deletion. func allDiffFuncs(funcs ...diffFunc) diffFunc { diff --git a/pkg/provision/sync/diff_test.go b/pkg/provision/sync/diff_test.go new file mode 100644 index 000000000..3790bb5b0 --- /dev/null +++ b/pkg/provision/sync/diff_test.go @@ -0,0 +1,219 @@ +// Copyright (c) 2019-2026 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package sync + +import ( + "reflect" + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestPodTemplateMetadataDiffFunc(t *testing.T) { + tests := []struct { + name string + specLabels map[string]string + specAnns map[string]string + clusterLabels map[string]string + clusterAnns map[string]string + expectUpdate bool + }{ + { + name: "no diff when labels match", + specLabels: map[string]string{"app": "test"}, + clusterLabels: map[string]string{"app": "test"}, + expectUpdate: false, + }, + { + name: "no diff when cluster has extra labels", + specLabels: map[string]string{"app": "test"}, + clusterLabels: map[string]string{"app": "test", "paas.redhat.com/appcode": "ITOS-123"}, + expectUpdate: false, + }, + { + name: "diff when spec label missing from cluster", + specLabels: map[string]string{"app": "test", "new-label": "value"}, + clusterLabels: map[string]string{"app": "test"}, + expectUpdate: true, + }, + { + name: "diff when spec label value differs", + specLabels: map[string]string{"app": "test-v2"}, + clusterLabels: map[string]string{"app": "test-v1"}, + expectUpdate: true, + }, + { + name: "no diff when cluster has extra annotations", + specAnns: map[string]string{"note": "hello"}, + clusterAnns: map[string]string{"note": "hello", "external.io/injected": "true"}, + expectUpdate: false, + }, + { + name: "diff when spec annotation missing from cluster", + specAnns: map[string]string{"note": "hello"}, + clusterAnns: map[string]string{}, + expectUpdate: true, + }, + { + name: "no diff with nil maps", + specLabels: nil, + clusterLabels: nil, + expectUpdate: false, + }, + { + name: "no diff when cluster has labels but spec has none", + specLabels: nil, + clusterLabels: map[string]string{"external": "label"}, + expectUpdate: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + spec := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: tt.specLabels, + Annotations: tt.specAnns, + }, + }, + }, + } + cluster := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: tt.clusterLabels, + Annotations: tt.clusterAnns, + }, + }, + }, + } + + _, update := podTemplateMetadataDiffFunc(spec, cluster) + if update != tt.expectUpdate { + t.Errorf("podTemplateMetadataDiffFunc() update = %v, want %v", update, tt.expectUpdate) + } + }) + } +} + +func TestPodTemplateMetadataDiffFunc_NonDeployment(t *testing.T) { + spec := &corev1.ConfigMap{} + cluster := &corev1.ConfigMap{} + shouldDelete, shouldUpdate := podTemplateMetadataDiffFunc(spec, cluster) + if shouldDelete || shouldUpdate { + t.Errorf("podTemplateMetadataDiffFunc() should return (false, false) for non-Deployment types, got (%v, %v)", shouldDelete, shouldUpdate) + } +} + +func TestDeploymentDiffOpts_IgnoresPodTemplateMetadata(t *testing.T) { + spec := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "test"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": "test"}, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "main", + Image: "test:latest", + }}, + }, + }, + }, + } + cluster := spec.DeepCopy() + cluster.Spec.Template.Labels["external.io/injected"] = "true" + + diffFn := basicDiffFunc(deploymentDiffOpts) + _, update := diffFn(spec, cluster) + if update { + t.Error("basicDiffFunc(deploymentDiffOpts) should not detect extra pod template labels as a diff") + } +} + +func TestDeploymentDiffOpts_DetectsSpecChanges(t *testing.T) { + spec := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "test"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": "test"}, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "main", + Image: "test:v2", + }}, + }, + }, + }, + } + cluster := spec.DeepCopy() + cluster.Spec.Template.Spec.Containers[0].Image = "test:v1" + + diffFn := basicDiffFunc(deploymentDiffOpts) + _, update := diffFn(spec, cluster) + if !update { + t.Error("basicDiffFunc(deploymentDiffOpts) should detect container image changes as a diff") + } +} + +func TestDeploymentFullDiff_ExternalLabelsNoUpdate(t *testing.T) { + specLabels := map[string]string{ + "controller.devfile.io/devworkspace_id": "workspace123", + "controller.devfile.io/devworkspace_name": "my-workspace", + } + spec := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Labels: specLabels, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "test"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "controller.devfile.io/devworkspace_id": "workspace123", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "main", + Image: "test:latest", + }}, + }, + }, + }, + } + cluster := spec.DeepCopy() + cluster.Labels["paas.redhat.com/appcode"] = "ITOS-123" + cluster.Spec.Template.Labels["paas.redhat.com/appcode"] = "ITOS-123" + + deploymentDiff := diffFuncs[reflect.TypeOf(appsv1.Deployment{})] + _, update := deploymentDiff(spec, cluster) + if update { + t.Error("deployment diff should not trigger update when only external labels are added to deployment and pod template metadata") + } +} diff --git a/pkg/provision/sync/diffopts.go b/pkg/provision/sync/diffopts.go index 536e82f77..5fc867d3d 100644 --- a/pkg/provision/sync/diffopts.go +++ b/pkg/provision/sync/diffopts.go @@ -39,6 +39,7 @@ var rolebindingDiffOpts = cmp.Options{ var deploymentDiffOpts = cmp.Options{ cmpopts.IgnoreFields(appsv1.Deployment{}, "TypeMeta", "ObjectMeta", "Status"), cmpopts.IgnoreFields(appsv1.DeploymentSpec{}, "RevisionHistoryLimit", "ProgressDeadlineSeconds"), + cmpopts.IgnoreFields(corev1.PodTemplateSpec{}, "ObjectMeta"), cmpopts.IgnoreFields(corev1.PodSpec{}, "DNSPolicy", "SchedulerName", "DeprecatedServiceAccount"), cmpopts.IgnoreFields(corev1.Container{}, "TerminationMessagePath", "TerminationMessagePolicy", "ImagePullPolicy"), cmpopts.SortSlices(func(a, b corev1.Container) bool { From 88421cd22fe2f97bdc30015f5c5ad4e0cec91843 Mon Sep 17 00:00:00 2001 From: David Kwon Date: Wed, 23 Sep 2026 20:03:20 -0400 Subject: [PATCH 2/5] Preserve external pod template labels during deployment updates Assisted-by: Claude Opus 4.6 Co-Authored-By: Claude Opus 4.6 Signed-off-by: David Kwon --- pkg/provision/sync/diff.go | 4 +- pkg/provision/sync/diffopts.go | 3 +- pkg/provision/sync/update.go | 27 +++++ pkg/provision/sync/update_test.go | 174 ++++++++++++++++++++++++++++++ 4 files changed, 205 insertions(+), 3 deletions(-) create mode 100644 pkg/provision/sync/update_test.go diff --git a/pkg/provision/sync/diff.go b/pkg/provision/sync/diff.go index ace79f0b5..7fac202e1 100644 --- a/pkg/provision/sync/diff.go +++ b/pkg/provision/sync/diff.go @@ -95,13 +95,13 @@ func podTemplateMetadataDiffFunc(spec, cluster crclient.Object) (delete, update clusterLabels := clusterDeploy.Spec.Template.Labels for k, v := range specDeploy.Spec.Template.Labels { - if clusterLabels[k] != v { + if cv, ok := clusterLabels[k]; !ok || cv != v { return false, true } } clusterAnnotations := clusterDeploy.Spec.Template.Annotations for k, v := range specDeploy.Spec.Template.Annotations { - if clusterAnnotations[k] != v { + if cv, ok := clusterAnnotations[k]; !ok || cv != v { return false, true } } diff --git a/pkg/provision/sync/diffopts.go b/pkg/provision/sync/diffopts.go index 5fc867d3d..afb8d5ea3 100644 --- a/pkg/provision/sync/diffopts.go +++ b/pkg/provision/sync/diffopts.go @@ -24,6 +24,7 @@ import ( corev1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) var roleDiffOpts = cmp.Options{ @@ -39,7 +40,7 @@ var rolebindingDiffOpts = cmp.Options{ var deploymentDiffOpts = cmp.Options{ cmpopts.IgnoreFields(appsv1.Deployment{}, "TypeMeta", "ObjectMeta", "Status"), cmpopts.IgnoreFields(appsv1.DeploymentSpec{}, "RevisionHistoryLimit", "ProgressDeadlineSeconds"), - cmpopts.IgnoreFields(corev1.PodTemplateSpec{}, "ObjectMeta"), + cmpopts.IgnoreFields(metav1.ObjectMeta{}, "Labels", "Annotations"), cmpopts.IgnoreFields(corev1.PodSpec{}, "DNSPolicy", "SchedulerName", "DeprecatedServiceAccount"), cmpopts.IgnoreFields(corev1.Container{}, "TerminationMessagePath", "TerminationMessagePolicy", "ImagePullPolicy"), cmpopts.SortSlices(func(a, b corev1.Container) bool { diff --git a/pkg/provision/sync/update.go b/pkg/provision/sync/update.go index f84a91d02..3a5b75d7c 100644 --- a/pkg/provision/sync/update.go +++ b/pkg/provision/sync/update.go @@ -17,6 +17,7 @@ import ( "errors" "reflect" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/client" crclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -67,9 +68,35 @@ func serviceAccountUpdateFunc(spec, cluster crclient.Object) (crclient.Object, e return spec, nil } +func deploymentUpdateFunc(spec, cluster crclient.Object) (crclient.Object, error) { + if cluster == nil { + return defaultUpdateFunc(spec, cluster) + } + specDeploy := spec.DeepCopyObject().(*appsv1.Deployment) + clusterDeploy := cluster.(*appsv1.Deployment) + specDeploy.ResourceVersion = clusterDeploy.ResourceVersion + + specDeploy.Spec.Template.Labels = mergeMaps(clusterDeploy.Spec.Template.Labels, specDeploy.Spec.Template.Labels) + specDeploy.Spec.Template.Annotations = mergeMaps(clusterDeploy.Spec.Template.Annotations, specDeploy.Spec.Template.Annotations) + return specDeploy, nil +} + +func mergeMaps(base, overlay map[string]string) map[string]string { + merged := make(map[string]string, len(base)+len(overlay)) + for k, v := range base { + merged[k] = v + } + for k, v := range overlay { + merged[k] = v + } + return merged +} + func getUpdateFunc(obj crclient.Object) updateFunc { objType := reflect.TypeOf(obj).Elem() switch objType { + case reflect.TypeOf(appsv1.Deployment{}): + return deploymentUpdateFunc case reflect.TypeOf(corev1.Service{}): return serviceUpdateFunc case reflect.TypeOf(corev1.ServiceAccount{}): diff --git a/pkg/provision/sync/update_test.go b/pkg/provision/sync/update_test.go new file mode 100644 index 000000000..8fa33db44 --- /dev/null +++ b/pkg/provision/sync/update_test.go @@ -0,0 +1,174 @@ +// Copyright (c) 2019-2026 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package sync + +import ( + "reflect" + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestGetUpdateFunc_Deployment(t *testing.T) { + deploy := &appsv1.Deployment{} + fn := getUpdateFunc(deploy) + defaultFn := getUpdateFunc(&corev1.ConfigMap{}) + + deployFnPtr := reflect.ValueOf(fn).Pointer() + defaultFnPtr := reflect.ValueOf(defaultFn).Pointer() + if deployFnPtr == defaultFnPtr { + t.Error("getUpdateFunc should return deploymentUpdateFunc for Deployments, not defaultUpdateFunc") + } +} + +func TestDeploymentUpdateFunc(t *testing.T) { + tests := []struct { + name string + specLabels map[string]string + specAnnotations map[string]string + clusterLabels map[string]string + clusterAnnotations map[string]string + expectedLabels map[string]string + expectedAnns map[string]string + }{ + { + name: "preserves externally-added pod template labels", + specLabels: map[string]string{"app": "test"}, + clusterLabels: map[string]string{"app": "test", "paas.redhat.com/appcode": "ITOS-123"}, + expectedLabels: map[string]string{"app": "test", "paas.redhat.com/appcode": "ITOS-123"}, + }, + { + name: "spec wins on conflict", + specLabels: map[string]string{"app": "new-value"}, + clusterLabels: map[string]string{"app": "old-value", "external": "keep"}, + expectedLabels: map[string]string{"app": "new-value", "external": "keep"}, + }, + { + name: "preserves externally-added pod template annotations", + specAnnotations: map[string]string{"note": "from-spec"}, + clusterAnnotations: map[string]string{"note": "from-spec", "injected": "by-webhook"}, + expectedAnns: map[string]string{"note": "from-spec", "injected": "by-webhook"}, + }, + { + name: "handles nil cluster labels", + specLabels: map[string]string{"app": "test"}, + clusterLabels: nil, + expectedLabels: map[string]string{"app": "test"}, + }, + { + name: "handles nil spec labels", + specLabels: nil, + clusterLabels: map[string]string{"external": "keep"}, + expectedLabels: map[string]string{"external": "keep"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + spec := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: tt.specLabels, + Annotations: tt.specAnnotations, + }, + }, + }, + } + cluster := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + ResourceVersion: "123", + }, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: tt.clusterLabels, + Annotations: tt.clusterAnnotations, + }, + }, + }, + } + + result, err := deploymentUpdateFunc(spec, cluster) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + resultDeploy := result.(*appsv1.Deployment) + if resultDeploy.ResourceVersion != "123" { + t.Errorf("expected ResourceVersion '123', got '%s'", resultDeploy.ResourceVersion) + } + + if tt.expectedLabels != nil { + if !reflect.DeepEqual(resultDeploy.Spec.Template.Labels, tt.expectedLabels) { + t.Errorf("labels mismatch:\n got: %v\n want: %v", resultDeploy.Spec.Template.Labels, tt.expectedLabels) + } + } + if tt.expectedAnns != nil { + if !reflect.DeepEqual(resultDeploy.Spec.Template.Annotations, tt.expectedAnns) { + t.Errorf("annotations mismatch:\n got: %v\n want: %v", resultDeploy.Spec.Template.Annotations, tt.expectedAnns) + } + } + }) + } +} + +func TestDeploymentUpdateFunc_NilCluster(t *testing.T) { + spec := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + } + result, err := deploymentUpdateFunc(spec, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.GetName() != "test" { + t.Errorf("expected name 'test', got '%s'", result.GetName()) + } +} + +func TestDeploymentUpdateFunc_DoesNotMutateInput(t *testing.T) { + spec := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": "test"}, + }, + }, + }, + } + cluster := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": "test", "external": "value"}, + }, + }, + }, + } + + _, err := deploymentUpdateFunc(spec, cluster) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if _, exists := spec.Spec.Template.Labels["external"]; exists { + t.Error("deploymentUpdateFunc should not mutate the input spec object") + } +} From c5c5f77c8a34fded0291534efd73265540df74e7 Mon Sep 17 00:00:00 2001 From: David Kwon Date: Wed, 23 Sep 2026 20:34:38 -0400 Subject: [PATCH 3/5] Fix diff comment accuracy and assert delete return value in tests Assisted-by: Claude Opus 4.6 Co-Authored-By: Claude Opus 4.6 Signed-off-by: David Kwon --- pkg/provision/sync/diff.go | 6 ++++-- pkg/provision/sync/diff_test.go | 20 ++++++++++++++++---- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/pkg/provision/sync/diff.go b/pkg/provision/sync/diff.go index 7fac202e1..92e121a83 100644 --- a/pkg/provision/sync/diff.go +++ b/pkg/provision/sync/diff.go @@ -84,8 +84,10 @@ func metadataDiffFunc(spec, cluster crclient.Object) (delete, update bool) { } // podTemplateMetadataDiffFunc requires a Deployment to be updated if any label or annotation present in the spec -// deployment's pod template is not present in the cluster deployment's pod template. Like metadataDiffFunc, it only -// checks the spec-to-cluster direction so that externally-added labels on the pod template do not trigger an update. +// deployment's pod template is missing from the cluster deployment's pod template or present with a different value. +// Like metadataDiffFunc, it only checks the spec-to-cluster direction so that externally-added labels on the pod +// template do not trigger an update. This is only safe because deploymentDiffOpts ignores PodTemplateSpec.ObjectMeta +// — see diffopts.go. func podTemplateMetadataDiffFunc(spec, cluster crclient.Object) (delete, update bool) { specDeploy, ok := spec.(*appsv1.Deployment) if !ok { diff --git a/pkg/provision/sync/diff_test.go b/pkg/provision/sync/diff_test.go index 3790bb5b0..62946fac4 100644 --- a/pkg/provision/sync/diff_test.go +++ b/pkg/provision/sync/diff_test.go @@ -104,7 +104,10 @@ func TestPodTemplateMetadataDiffFunc(t *testing.T) { }, } - _, update := podTemplateMetadataDiffFunc(spec, cluster) + del, update := podTemplateMetadataDiffFunc(spec, cluster) + if del { + t.Errorf("podTemplateMetadataDiffFunc() delete = true, want false") + } if update != tt.expectUpdate { t.Errorf("podTemplateMetadataDiffFunc() update = %v, want %v", update, tt.expectUpdate) } @@ -144,7 +147,10 @@ func TestDeploymentDiffOpts_IgnoresPodTemplateMetadata(t *testing.T) { cluster.Spec.Template.Labels["external.io/injected"] = "true" diffFn := basicDiffFunc(deploymentDiffOpts) - _, update := diffFn(spec, cluster) + del, update := diffFn(spec, cluster) + if del { + t.Error("basicDiffFunc(deploymentDiffOpts) delete = true, want false") + } if update { t.Error("basicDiffFunc(deploymentDiffOpts) should not detect extra pod template labels as a diff") } @@ -173,7 +179,10 @@ func TestDeploymentDiffOpts_DetectsSpecChanges(t *testing.T) { cluster.Spec.Template.Spec.Containers[0].Image = "test:v1" diffFn := basicDiffFunc(deploymentDiffOpts) - _, update := diffFn(spec, cluster) + del, update := diffFn(spec, cluster) + if del { + t.Error("basicDiffFunc(deploymentDiffOpts) delete = true, want false") + } if !update { t.Error("basicDiffFunc(deploymentDiffOpts) should detect container image changes as a diff") } @@ -212,7 +221,10 @@ func TestDeploymentFullDiff_ExternalLabelsNoUpdate(t *testing.T) { cluster.Spec.Template.Labels["paas.redhat.com/appcode"] = "ITOS-123" deploymentDiff := diffFuncs[reflect.TypeOf(appsv1.Deployment{})] - _, update := deploymentDiff(spec, cluster) + del, update := deploymentDiff(spec, cluster) + if del { + t.Error("deployment diff should not trigger delete when only external labels are added to deployment and pod template metadata") + } if update { t.Error("deployment diff should not trigger update when only external labels are added to deployment and pod template metadata") } From 4ece3f93155cdfafa9e92cb38506bc42b5c2c251 Mon Sep 17 00:00:00 2001 From: David Kwon Date: Thu, 24 Sep 2026 11:45:27 -0400 Subject: [PATCH 4/5] Update test to verify that the resulting deployment update function is working as expected Signed-off-by: David Kwon --- pkg/provision/sync/update_test.go | 110 ++++++++++++++++-------------- 1 file changed, 58 insertions(+), 52 deletions(-) diff --git a/pkg/provision/sync/update_test.go b/pkg/provision/sync/update_test.go index 8fa33db44..7314e983b 100644 --- a/pkg/provision/sync/update_test.go +++ b/pkg/provision/sync/update_test.go @@ -32,35 +32,69 @@ func TestGetUpdateFunc_Deployment(t *testing.T) { if deployFnPtr == defaultFnPtr { t.Error("getUpdateFunc should return deploymentUpdateFunc for Deployments, not defaultUpdateFunc") } + + spec := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": "test"}, + }, + }, + }, + } + cluster := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + ResourceVersion: "456", + }, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": "test", "paas.redhat.com/appcode": "ITOS-123"}, + }, + }, + }, + } + + result, err := fn(spec, cluster) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + resultDeploy := result.(*appsv1.Deployment) + expectedLabels := map[string]string{"app": "test"} + if !reflect.DeepEqual(resultDeploy.Spec.Template.Labels, expectedLabels) { + t.Errorf("pod template labels mismatch:\n got: %v\n want: %v", resultDeploy.Spec.Template.Labels, expectedLabels) + } } func TestDeploymentUpdateFunc(t *testing.T) { tests := []struct { - name string - specLabels map[string]string - specAnnotations map[string]string - clusterLabels map[string]string - clusterAnnotations map[string]string - expectedLabels map[string]string - expectedAnns map[string]string + name string + specLabels map[string]string + specAnnotations map[string]string + clusterLabels map[string]string + clusterAnns map[string]string + expectedLabels map[string]string + expectedAnns map[string]string }{ { - name: "preserves externally-added pod template labels", + name: "spec labels replace cluster labels", specLabels: map[string]string{"app": "test"}, clusterLabels: map[string]string{"app": "test", "paas.redhat.com/appcode": "ITOS-123"}, - expectedLabels: map[string]string{"app": "test", "paas.redhat.com/appcode": "ITOS-123"}, + expectedLabels: map[string]string{"app": "test"}, }, { name: "spec wins on conflict", specLabels: map[string]string{"app": "new-value"}, clusterLabels: map[string]string{"app": "old-value", "external": "keep"}, - expectedLabels: map[string]string{"app": "new-value", "external": "keep"}, + expectedLabels: map[string]string{"app": "new-value"}, }, { - name: "preserves externally-added pod template annotations", - specAnnotations: map[string]string{"note": "from-spec"}, - clusterAnnotations: map[string]string{"note": "from-spec", "injected": "by-webhook"}, - expectedAnns: map[string]string{"note": "from-spec", "injected": "by-webhook"}, + name: "spec annotations replace cluster annotations", + specAnnotations: map[string]string{"note": "from-spec"}, + clusterAnns: map[string]string{"note": "from-spec", "injected": "by-webhook"}, + expectedAnns: map[string]string{"note": "from-spec"}, }, { name: "handles nil cluster labels", @@ -72,7 +106,13 @@ func TestDeploymentUpdateFunc(t *testing.T) { name: "handles nil spec labels", specLabels: nil, clusterLabels: map[string]string{"external": "keep"}, - expectedLabels: map[string]string{"external": "keep"}, + expectedLabels: nil, + }, + { + name: "removed spec label does not persist from cluster", + specLabels: map[string]string{"app": "test"}, + clusterLabels: map[string]string{"app": "test", "env": "staging"}, + expectedLabels: map[string]string{"app": "test"}, }, } @@ -98,7 +138,7 @@ func TestDeploymentUpdateFunc(t *testing.T) { Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: tt.clusterLabels, - Annotations: tt.clusterAnnotations, + Annotations: tt.clusterAnns, }, }, }, @@ -114,10 +154,8 @@ func TestDeploymentUpdateFunc(t *testing.T) { t.Errorf("expected ResourceVersion '123', got '%s'", resultDeploy.ResourceVersion) } - if tt.expectedLabels != nil { - if !reflect.DeepEqual(resultDeploy.Spec.Template.Labels, tt.expectedLabels) { - t.Errorf("labels mismatch:\n got: %v\n want: %v", resultDeploy.Spec.Template.Labels, tt.expectedLabels) - } + if !reflect.DeepEqual(resultDeploy.Spec.Template.Labels, tt.expectedLabels) { + t.Errorf("labels mismatch:\n got: %v\n want: %v", resultDeploy.Spec.Template.Labels, tt.expectedLabels) } if tt.expectedAnns != nil { if !reflect.DeepEqual(resultDeploy.Spec.Template.Annotations, tt.expectedAnns) { @@ -140,35 +178,3 @@ func TestDeploymentUpdateFunc_NilCluster(t *testing.T) { t.Errorf("expected name 'test', got '%s'", result.GetName()) } } - -func TestDeploymentUpdateFunc_DoesNotMutateInput(t *testing.T) { - spec := &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{Name: "test"}, - Spec: appsv1.DeploymentSpec{ - Template: corev1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{"app": "test"}, - }, - }, - }, - } - cluster := &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, - Spec: appsv1.DeploymentSpec{ - Template: corev1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{"app": "test", "external": "value"}, - }, - }, - }, - } - - _, err := deploymentUpdateFunc(spec, cluster) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - - if _, exists := spec.Spec.Template.Labels["external"]; exists { - t.Error("deploymentUpdateFunc should not mutate the input spec object") - } -} From ffaed317802411cae422676cb87dbfd3f6e5cd94 Mon Sep 17 00:00:00 2001 From: David Kwon Date: Thu, 24 Sep 2026 12:46:46 -0400 Subject: [PATCH 5/5] Update formatting Signed-off-by: David Kwon --- .../crd/bases/controller.devfile.io_devworkspaceroutings.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/deploy/templates/crd/bases/controller.devfile.io_devworkspaceroutings.yaml b/deploy/templates/crd/bases/controller.devfile.io_devworkspaceroutings.yaml index 025b4c637..ed4178ecb 100644 --- a/deploy/templates/crd/bases/controller.devfile.io_devworkspaceroutings.yaml +++ b/deploy/templates/crd/bases/controller.devfile.io_devworkspaceroutings.yaml @@ -1,3 +1,4 @@ +--- apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: