OCPBUGS-17113: address reviews

* check for spec changes before metadata ones, and add a comment
  explaining why.
* add a unit test for `hashOfResourceStruct`.

Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

pkg/operator/resource/resourceapply/monitoring.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	errorsstdlib "errors"
 	"fmt"
+	"github.com/openshift/library-go/pkg/operator/resource/resourcemerge"
 	"k8s.io/apimachinery/pkg/api/equality"
 
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -15,7 +16,6 @@ import (
 	"k8s.io/klog/v2"
 
 	"github.com/openshift/library-go/pkg/operator/events"
-	"github.com/openshift/library-go/pkg/operator/resource/resourcemerge"
 )
 
 var alertmanagerGVR = schema.GroupVersionResource{Group: "monitoring.coreos.com", Version: "v1", Resource: "alertmanagers"}
@@ -116,16 +116,6 @@ func ApplyUnstructuredResourceImproved(ctx context.Context, client dynamic.Inter
 		return nil, false, err
 	}
 
-	// Check if the metadata objects differ.
-	modifiedPtr := resourcemerge.BoolPtr(false)
-	resourcemerge.EnsureObjectMeta(modifiedPtr, &existingObjectMetaTyped, requiredObjectMetaTyped)
-	if !*modifiedPtr {
-		// Update cache even if certain fields are not modified, in order to maintain a consistent cache based on the
-		// resource hash. The resource hash depends on the entire metadata, not just the fields that were checked above,
-		cache.UpdateCachedResourceMetadata(required, existingCopy)
-		return existingCopy, false, nil
-	}
-
 	// Deep-check the spec objects for equality, and update the cache in either case.
 	existingCopy, modified, err := ensureGenericSpec(required, existingCopy, noDefaulting, equality.Semantic)
 	if err != nil {
@@ -138,6 +128,19 @@ func ApplyUnstructuredResourceImproved(ctx context.Context, client dynamic.Inter
 		return existingCopy, false, nil
 	}
 
+	// Check if the metadata objects differ.
+	// NOTE: This is done after the spec check to detect obvious spec changes first (return early), and the fact that
+	// resourcemerge.EnsureObjectMeta compares a subset of metadata fields (which is why we update the cache even if no
+	// metadata modifications are detected).
+	modifiedPtr := resourcemerge.BoolPtr(false)
+	resourcemerge.EnsureObjectMeta(modifiedPtr, &existingObjectMetaTyped, requiredObjectMetaTyped)
+	if !*modifiedPtr {
+		// Update cache even if certain fields are not modified, in order to maintain a consistent cache based on the
+		// resource hash. The resource hash depends on the entire metadata, not just the fields that were checked above,
+		cache.UpdateCachedResourceMetadata(required, existingCopy)
+		return existingCopy, false, nil
+	}
+
 	if klog.V(4).Enabled() {
 		klog.Infof("%s %q changes: %v", resourceGVR.String(), namespace+"/"+name, JSONPatchNoError(existing, existingCopy))
 	}

pkg/operator/resource/resourceapply/resource_cache_test.go

@@ -0,0 +1,36 @@
+package resourceapply
+
+import (
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"testing"
+)
+
+func TestHashOfResourceStructUnstructured(t *testing.T) {
+	unstructuredObject := unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"kind":       "Service",
+			"apiVersion": "v1",
+			"metadata": map[string]interface{}{
+				"name":      "svc1",
+				"namespace": "ns1",
+			},
+			"spec": map[string]interface{}{
+				"selector": map[string]interface{}{
+					"foo": "bar",
+				},
+				"ports": []map[string]interface{}{
+					{
+						"protocol":   "TCP",
+						"port":       80,
+						"targetPort": 9376,
+					},
+				},
+			},
+		},
+	}
+	hash := hashOfResourceStruct(&unstructuredObject)
+	unstructuredObject.Object["spec"].(map[string]interface{})["selector"].(map[string]interface{})["foo"] = "baz"
+	if hashOfResourceStruct(&unstructuredObject) == hash {
+		t.Errorf("expected a different hash after modifying the object")
+	}
+}