Skip to content

Commit d27edb4

Browse files
rexagodrexagod
committed
enhancement: drop the redundant ApplyUnstructuredResourceImproved return value
Drop the boolean return value that signified if an update happened as it is not necessary and pollutes the signature. Signed-off-by: Pranshu Srivastava <[email protected]>
1 parent af5b21e commit d27edb4

File tree

2 files changed

+95
-25
lines changed

2 files changed

+95
-25
lines changed

‎pkg/operator/resource/resourceapply/monitoring.go

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ var serviceMonitorGVR = schema.GroupVersionResource{Group: "monitoring.coreos.co
2222

2323
// ApplyAlertmanager applies the Alertmanager.
2424
func ApplyAlertmanager(ctx context.Context, client dynamic.Interface, recorder events.Recorder, required *unstructured.Unstructured) (*unstructured.Unstructured, bool, error) {
25-
return ApplyUnstructuredResourceImproved(ctx, client, recorder, required, noCache, alertmanagerGVR, nil, nil)
25+
return ApplyUnstructuredResourceImprovedDeprecated(ctx, client, recorder, required, noCache, alertmanagerGVR, nil, nil)
2626
}
2727

2828
// DeleteAlertmanager deletes the Alertmanager.
@@ -32,7 +32,7 @@ func DeleteAlertmanager(ctx context.Context, client dynamic.Interface, recorder
3232

3333
// ApplyPrometheus applies the Prometheus.
3434
func ApplyPrometheus(ctx context.Context, client dynamic.Interface, recorder events.Recorder, required *unstructured.Unstructured) (*unstructured.Unstructured, bool, error) {
35-
return ApplyUnstructuredResourceImproved(ctx, client, recorder, required, noCache, prometheusGVR, nil, nil)
35+
return ApplyUnstructuredResourceImprovedDeprecated(ctx, client, recorder, required, noCache, prometheusGVR, nil, nil)
3636
}
3737

3838
// DeletePrometheus deletes the Prometheus.
@@ -42,7 +42,7 @@ func DeletePrometheus(ctx context.Context, client dynamic.Interface, recorder ev
4242

4343
// ApplyPrometheusRule applies the PrometheusRule.
4444
func ApplyPrometheusRule(ctx context.Context, client dynamic.Interface, recorder events.Recorder, required *unstructured.Unstructured) (*unstructured.Unstructured, bool, error) {
45-
return ApplyUnstructuredResourceImproved(ctx, client, recorder, required, noCache, prometheusRuleGVR, nil, nil)
45+
return ApplyUnstructuredResourceImprovedDeprecated(ctx, client, recorder, required, noCache, prometheusRuleGVR, nil, nil)
4646
}
4747

4848
// DeletePrometheusRule deletes the PrometheusRule.
@@ -52,7 +52,7 @@ func DeletePrometheusRule(ctx context.Context, client dynamic.Interface, recorde
5252

5353
// ApplyServiceMonitor applies the ServiceMonitor.
5454
func ApplyServiceMonitor(ctx context.Context, client dynamic.Interface, recorder events.Recorder, required *unstructured.Unstructured) (*unstructured.Unstructured, bool, error) {
55-
return ApplyUnstructuredResourceImproved(ctx, client, recorder, required, noCache, serviceMonitorGVR, nil, nil)
55+
return ApplyUnstructuredResourceImprovedDeprecated(ctx, client, recorder, required, noCache, serviceMonitorGVR, nil, nil)
5656
}
5757

5858
// DeleteServiceMonitor deletes the ServiceMonitor.
@@ -72,6 +72,26 @@ func ApplyUnstructuredResourceImproved(
7272
resourceGVR schema.GroupVersionResource,
7373
defaultingFunc mimicDefaultingFunc,
7474
equalityChecker equalityChecker,
75+
) (*unstructured.Unstructured, error) {
76+
gotUnstructured, _, err := ApplyUnstructuredResourceImprovedDeprecated(ctx, client, recorder, required, cache, resourceGVR, defaultingFunc, equalityChecker)
77+
return gotUnstructured, err
78+
}
79+
80+
// Deprecated: Use ApplyUnstructuredResourceImproved instead.
81+
// NOTE: The return values (excluding the *unstructured.Unstructured one) establish the following matrix (w.r.t. the create or update verbs):
82+
// * true, nil : verb action needed; operation successful
83+
// * false, nil : verb action not needed; operation skipped
84+
// * true, error : verb action needed, operation unsuccessful
85+
// * false, error: verb action may or may not be needed; operation unsuccessful
86+
func ApplyUnstructuredResourceImprovedDeprecated(
87+
ctx context.Context,
88+
client dynamic.Interface,
89+
recorder events.Recorder,
90+
required *unstructured.Unstructured,
91+
cache ResourceCache,
92+
resourceGVR schema.GroupVersionResource,
93+
defaultingFunc mimicDefaultingFunc,
94+
equalityChecker equalityChecker,
7595
) (*unstructured.Unstructured, bool, error) {
7696
name := required.GetName()
7797
namespace := required.GetNamespace()

‎test/e2e-monitoring/monitoring_test.go

Lines changed: 71 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,12 @@ package e2e_monitoring
33
import (
44
"context"
55
clocktesting "k8s.io/utils/clock/testing"
6+
"os"
67
"testing"
78
"time"
89

910
"github.com/openshift/library-go/pkg/operator/events"
1011
"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
11-
"github.com/openshift/library-go/test/library"
1212
monv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
1313
"github.com/stretchr/testify/require"
1414
"k8s.io/apimachinery/pkg/api/errors"
@@ -18,10 +18,45 @@ import (
1818
"k8s.io/apimachinery/pkg/runtime/schema"
1919
"k8s.io/apimachinery/pkg/util/intstr"
2020
"k8s.io/client-go/dynamic"
21+
"k8s.io/client-go/tools/clientcmd"
2122
)
2223

24+
const (
25+
noUpdate = iota
26+
metadataUpdate
27+
specAndMaybeMetadataUpdate
28+
)
29+
30+
// didUpdate compares two unstructured resources and returns if the specified parts changed.
31+
func didUpdate(old, new *unstructured.Unstructured) (int, error) {
32+
oldResourceVersion, _, err := unstructured.NestedString(old.Object, "metadata", "resourceVersion")
33+
if err != nil {
34+
return 0, err
35+
}
36+
newResourceVersion, _, err := unstructured.NestedString(new.Object, "metadata", "resourceVersion")
37+
if err != nil {
38+
return 0, err
39+
}
40+
oldGeneration, _, err := unstructured.NestedInt64(old.Object, "metadata", "generation")
41+
if err != nil {
42+
return 0, err
43+
}
44+
newGeneration, _, err := unstructured.NestedInt64(new.Object, "metadata", "generation")
45+
if err != nil {
46+
return 0, err
47+
}
48+
if oldResourceVersion != newResourceVersion {
49+
if oldGeneration != newGeneration {
50+
return specAndMaybeMetadataUpdate, nil
51+
}
52+
return metadataUpdate, nil
53+
}
54+
55+
return noUpdate, nil
56+
}
57+
2358
func TestResourceVersionApplication(t *testing.T) {
24-
config, err := library.NewClientConfigForTest()
59+
config, err := clientcmd.BuildConfigFromFlags("", os.Getenv("KUBECONFIG"))
2560
require.NoError(t, err)
2661

2762
// Define the resource.
@@ -70,7 +105,8 @@ func TestResourceVersionApplication(t *testing.T) {
70105
unstructuredResourceMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&resource)
71106
require.NoError(t, err)
72107
unstructuredResource.SetUnstructuredContent(unstructuredResourceMap)
73-
gotUnstructured, didUpdate, err := resourceapply.ApplyUnstructuredResourceImproved(
108+
oldUnstructuredResource := unstructuredResource.DeepCopy()
109+
gotUnstructuredResource, err := resourceapply.ApplyUnstructuredResourceImproved(
74110
context.TODO(),
75111
dynamicClient,
76112
recorder,
@@ -83,15 +119,19 @@ func TestResourceVersionApplication(t *testing.T) {
83119
if err != nil && !errors.IsAlreadyExists(err) {
84120
t.Fatalf("Failed to create resource: %v", err)
85121
}
86-
require.True(t, didUpdate)
122+
expectation, err := didUpdate(oldUnstructuredResource, gotUnstructuredResource)
123+
if err != nil {
124+
t.Fatalf("Failed to compare resources: %v", err)
125+
}
126+
require.True(t, expectation != noUpdate)
87127

88128
// Update the resource version and the generation since we made a spec change.
89-
unstructuredResource.SetResourceVersion(gotUnstructured.GetResourceVersion())
90-
unstructuredResource.SetGeneration(gotUnstructured.GetGeneration())
91-
unstructuredResource.SetCreationTimestamp(gotUnstructured.GetCreationTimestamp())
92-
unstructuredResource.SetUID(gotUnstructured.GetUID())
93-
unstructuredResource.SetManagedFields(gotUnstructured.GetManagedFields())
94-
require.Equal(t, unstructuredResource.UnstructuredContent(), gotUnstructured.UnstructuredContent())
129+
unstructuredResource.SetResourceVersion(gotUnstructuredResource.GetResourceVersion())
130+
unstructuredResource.SetGeneration(gotUnstructuredResource.GetGeneration())
131+
unstructuredResource.SetCreationTimestamp(gotUnstructuredResource.GetCreationTimestamp())
132+
unstructuredResource.SetUID(gotUnstructuredResource.GetUID())
133+
unstructuredResource.SetManagedFields(gotUnstructuredResource.GetManagedFields())
134+
require.Equal(t, unstructuredResource.UnstructuredContent(), gotUnstructuredResource.UnstructuredContent())
95135

96136
// Compare the existing resource with the one we have.
97137
existingResourceUnstructured, err := dynamicClient.Resource(gvr).Namespace(resource.GetNamespace()).Get(context.TODO(), resource.GetName(), metav1.GetOptions{})
@@ -103,7 +143,8 @@ func TestResourceVersionApplication(t *testing.T) {
103143
unstructuredResourceMap, err = runtime.DefaultUnstructuredConverter.ToUnstructured(&resource)
104144
require.NoError(t, err)
105145
unstructuredResource.SetUnstructuredContent(unstructuredResourceMap)
106-
gotUnstructured, didUpdate, err = resourceapply.ApplyUnstructuredResourceImproved(
146+
oldUnstructuredResource = gotUnstructuredResource.DeepCopy()
147+
gotUnstructuredResource, err = resourceapply.ApplyUnstructuredResourceImproved(
107148
context.TODO(),
108149
dynamicClient,
109150
recorder,
@@ -116,23 +157,28 @@ func TestResourceVersionApplication(t *testing.T) {
116157
if err != nil {
117158
t.Fatalf("Failed to update resource: %v", err)
118159
}
119-
require.True(t, didUpdate)
160+
expectation, err = didUpdate(oldUnstructuredResource, gotUnstructuredResource)
161+
if err != nil {
162+
t.Fatalf("Failed to compare resources: %v", err)
163+
}
164+
require.True(t, expectation == specAndMaybeMetadataUpdate)
120165

121166
// Update the resource version and the generation since we made a spec change.
122-
unstructuredResource.SetResourceVersion(gotUnstructured.GetResourceVersion())
123-
unstructuredResource.SetGeneration(gotUnstructured.GetGeneration())
124-
unstructuredResource.SetCreationTimestamp(gotUnstructured.GetCreationTimestamp())
125-
unstructuredResource.SetUID(gotUnstructured.GetUID())
126-
unstructuredResource.SetManagedFields(gotUnstructured.GetManagedFields())
127-
require.Equal(t, unstructuredResource.UnstructuredContent(), gotUnstructured.UnstructuredContent())
167+
unstructuredResource.SetResourceVersion(gotUnstructuredResource.GetResourceVersion())
168+
unstructuredResource.SetGeneration(gotUnstructuredResource.GetGeneration())
169+
unstructuredResource.SetCreationTimestamp(gotUnstructuredResource.GetCreationTimestamp())
170+
unstructuredResource.SetUID(gotUnstructuredResource.GetUID())
171+
unstructuredResource.SetManagedFields(gotUnstructuredResource.GetManagedFields())
172+
require.Equal(t, unstructuredResource.UnstructuredContent(), gotUnstructuredResource.UnstructuredContent())
128173

129174
// Compare the existing resource with the one we have.
130175
existingResourceUnstructured, err = dynamicClient.Resource(gvr).Namespace(resource.GetNamespace()).Get(context.TODO(), resource.GetName(), metav1.GetOptions{})
131176
require.NoError(t, err)
132177
require.Equal(t, unstructuredResource.UnstructuredContent(), existingResourceUnstructured.UnstructuredContent())
133178

134179
// Update the resource without any changes, without specifying a resource version.
135-
gotUnstructured, didUpdate, err = resourceapply.ApplyUnstructuredResourceImproved(
180+
oldUnstructuredResource = gotUnstructuredResource.DeepCopy()
181+
gotUnstructuredResource, err = resourceapply.ApplyUnstructuredResourceImproved(
136182
context.TODO(),
137183
dynamicClient,
138184
recorder,
@@ -145,10 +191,14 @@ func TestResourceVersionApplication(t *testing.T) {
145191
if err != nil {
146192
t.Fatalf("Failed to update resource: %v", err)
147193
}
148-
require.False(t, didUpdate)
194+
expectation, err = didUpdate(oldUnstructuredResource, gotUnstructuredResource)
195+
if err != nil {
196+
t.Fatalf("Failed to compare resources: %v", err)
197+
}
198+
require.True(t, expectation == noUpdate)
149199

150200
// Do not update any fields as no change was made.
151-
require.Equal(t, unstructuredResource.UnstructuredContent(), gotUnstructured.UnstructuredContent())
201+
require.Equal(t, unstructuredResource.UnstructuredContent(), gotUnstructuredResource.UnstructuredContent())
152202

153203
// Compare the existing resource with the one we have.
154204
existingResourceUnstructured, err = dynamicClient.Resource(gvr).Namespace(resource.GetNamespace()).Get(context.TODO(), resource.GetName(), metav1.GetOptions{})

0 commit comments

Comments
 (0)