inventory client replace skipped in dry-run

Author: seans3

go.sum

@@ -703,8 +703,6 @@ sigs.k8s.io/controller-runtime v0.6.0 h1:Fzna3DY7c4BIP6KwfSlrfnj20DJ+SeMBK8HSFvO
 sigs.k8s.io/controller-runtime v0.6.0/go.mod h1:CpYf5pdNY/B352A1TFLAS2JVSlnGQ5O2cftPHndTroo=
 sigs.k8s.io/kustomize v2.0.3+incompatible h1:JUufWFNlI44MdtnjUqVnvh29rR37PQFzPbLXqhyOyX0=
 sigs.k8s.io/kustomize v2.0.3+incompatible/go.mod h1:MkjgH3RdOWrievjo6c9T245dYlB5QeXV4WCbnt/PEpU=
-sigs.k8s.io/kustomize/kyaml v0.9.3 h1:kZ5HnNmmnbndSXFivrAVM6u3Ik1dwu4kbpaV8KNwy8I=
-sigs.k8s.io/kustomize/kyaml v0.9.3/go.mod h1:UTm64bSWVdBUA8EQoYCxVOaBQxUdIOr5LKWxA4GNbkw=
 sigs.k8s.io/kustomize/kyaml v0.9.4 h1:DDuzZtjIzFqp2IPy4DTyCI69Cl3bDgcJODjI6sjF9NY=
 sigs.k8s.io/kustomize/kyaml v0.9.4/go.mod h1:UTm64bSWVdBUA8EQoYCxVOaBQxUdIOr5LKWxA4GNbkw=
 sigs.k8s.io/structured-merge-diff/v3 v3.0.0-20200116222232-67a7b8c61874/go.mod h1:PlARxl6Hbt/+BC80dRLi1qAmnMqwqDg62YvvVkZjemw=

pkg/inventory/inventory-client.go

@@ -151,6 +151,11 @@ func (cic *ClusterInventoryClient) Merge(localInv InventoryInfo, objs []object.O
 // Replace stores the passed objects in the cluster inventory object, or
 // an error if one occurred.
 func (cic *ClusterInventoryClient) Replace(localInv InventoryInfo, objs []object.ObjMetadata) error {
+	// Skip entire function for dry-run.
+	if cic.dryRunStrategy.ClientOrServerDryRun() {
+		klog.V(4).Infoln("dry-run replace inventory object: not applied")
+		return nil
+	}
 	clusterObjs, err := cic.GetClusterObjs(localInv)
 	if err != nil {
 		return err
@@ -163,24 +168,31 @@ func (cic *ClusterInventoryClient) Replace(localInv InventoryInfo, objs []object
 	if err != nil {
 		return err
 	}
-	wrappedInv := cic.InventoryFactoryFunc(clusterInv)
-	if err = wrappedInv.Store(objs); err != nil {
+	clusterInv, err = cic.replaceInventory(clusterInv, objs)
+	if err != nil {
 		return err
 	}
-	if !cic.dryRunStrategy.ClientOrServerDryRun() {
-		clusterInv, err = wrappedInv.GetObject()
-		if err != nil {
-			return err
-		}
-		klog.V(4).Infof("replace cluster inventory: %s/%s", clusterInv.GetNamespace(), clusterInv.GetName())
-		klog.V(4).Infof("replace cluster inventory %d objects", len(objs))
-		if err := cic.applyInventoryObj(clusterInv); err != nil {
-			return err
-		}
+	klog.V(4).Infof("replace cluster inventory: %s/%s", clusterInv.GetNamespace(), clusterInv.GetName())
+	klog.V(4).Infof("replace cluster inventory %d objects", len(objs))
+	if err := cic.applyInventoryObj(clusterInv); err != nil {
+		return err
 	}
 	return nil
 }
 
+// replaceInventory stores the passed objects into the passed inventory object.
+func (cic *ClusterInventoryClient) replaceInventory(inv *unstructured.Unstructured, objs []object.ObjMetadata) (*unstructured.Unstructured, error) {
+	wrappedInv := cic.InventoryFactoryFunc(inv)
+	if err := wrappedInv.Store(objs); err != nil {
+		return nil, err
+	}
+	clusterInv, err := wrappedInv.GetObject()
+	if err != nil {
+		return nil, err
+	}
+	return clusterInv, nil
+}
+
 // DeleteInventoryObj deletes the inventory object from the cluster.
 func (cic *ClusterInventoryClient) DeleteInventoryObj(localInv InventoryInfo) error {
 	return cic.deleteInventoryObj(cic.invToUnstructuredFunc(localInv))

pkg/inventory/inventory-client_test.go

@@ -268,85 +268,85 @@ func TestCreateInventory(t *testing.T) {
 
 func TestReplace(t *testing.T) {
 	tests := map[string]struct {
-		localInv    InventoryInfo
 		localObjs   []object.ObjMetadata
 		clusterObjs []object.ObjMetadata
-		isError     bool
 	}{
-		"Local inventory nil is error": {
-			localInv:    nil,
+		"Cluster and local inventories empty": {
 			localObjs:   []object.ObjMetadata{},
 			clusterObjs: []object.ObjMetadata{},
-			isError:     true,
 		},
-		"Cluster and local inventories empty: no error": {
-			localInv:    copyInventory(),
-			localObjs:   []object.ObjMetadata{},
-			clusterObjs: []object.ObjMetadata{},
-			isError:     false,
-		},
-		"Cluster and local inventories same: no error": {
-			localInv: copyInventory(),
+		"Cluster and local inventories same": {
 			localObjs: []object.ObjMetadata{
 				ignoreErrInfoToObjMeta(pod1Info),
 			},
 			clusterObjs: []object.ObjMetadata{
 				ignoreErrInfoToObjMeta(pod1Info),
 			},
-			isError: false,
 		},
-		"Cluster two obj, local one: no error": {
-			localInv: copyInventory(),
+		"Cluster two obj, local one": {
 			localObjs: []object.ObjMetadata{
 				ignoreErrInfoToObjMeta(pod1Info),
 			},
 			clusterObjs: []object.ObjMetadata{
 				ignoreErrInfoToObjMeta(pod1Info),
 				ignoreErrInfoToObjMeta(pod3Info),
 			},
-			isError: false,
 		},
-		"Cluster multiple objs, local multiple different objs: no error": {
-			localInv: copyInventory(),
+		"Cluster multiple objs, local multiple different objs": {
 			localObjs: []object.ObjMetadata{
 				ignoreErrInfoToObjMeta(pod2Info),
 			},
 			clusterObjs: []object.ObjMetadata{
 				ignoreErrInfoToObjMeta(pod1Info),
 				ignoreErrInfoToObjMeta(pod2Info),
 				ignoreErrInfoToObjMeta(pod3Info)},
-			isError: false,
 		},
 	}
 
 	tf := cmdtesting.NewTestFactory().WithNamespace(testNamespace)
 	defer tf.Cleanup()
 
+	// Client and server dry-run do not throw errors.
+	invClient, _ := NewInventoryClient(tf, WrapInventoryObj, InvInfoToConfigMap)
+	invClient.SetDryRunStrategy(common.DryRunClient)
+	err := invClient.Replace(copyInventory(), []object.ObjMetadata{})
+	if err != nil {
+		t.Fatalf("unexpected error received: %s", err)
+	}
+	invClient.SetDryRunStrategy(common.DryRunServer)
+	err = invClient.Replace(copyInventory(), []object.ObjMetadata{})
+	if err != nil {
+		t.Fatalf("unexpected error received: %s", err)
+	}
+
 	for name, tc := range tests {
-		for i := range common.Strategies {
-			drs := common.Strategies[i]
-			t.Run(name, func(t *testing.T) {
-				invClient, _ := NewInventoryClient(tf,
-					WrapInventoryObj, InvInfoToConfigMap)
-				invClient.SetDryRunStrategy(drs)
-				// Create fake builder returning the cluster inventory object
-				// storing the "tc.clusterObjs" objects.
-				fakeBuilder := FakeBuilder{}
-				fakeBuilder.SetInventoryObjs(tc.clusterObjs)
-				invClient.builderFunc = fakeBuilder.GetBuilder()
-				// Call "Replace", passing in the new local inventory objects
-				err := invClient.Replace(tc.localInv, tc.localObjs)
-				if tc.isError {
-					if err == nil {
-						t.Fatalf("expected error but received none")
-					}
-					return
-				}
-				if !tc.isError && err != nil {
-					t.Fatalf("unexpected error received: %s", err)
-				}
-			})
-		}
+		t.Run(name, func(t *testing.T) {
+			// Create inventory client, and store the cluster objs in the inventory object.
+			invClient, _ := NewInventoryClient(tf,
+				WrapInventoryObj, InvInfoToConfigMap)
+			wrappedInv := invClient.InventoryFactoryFunc(inventoryObj)
+			if err := wrappedInv.Store(tc.clusterObjs); err != nil {
+				t.Fatalf("unexpected error storing inventory objects: %s", err)
+			}
+			inv, err := wrappedInv.GetObject()
+			if err != nil {
+				t.Fatalf("unexpected error storing inventory objects: %s", err)
+			}
+			// Call replaceInventory with the new set of "localObjs"
+			inv, err = invClient.replaceInventory(inv, tc.localObjs)
+			if err != nil {
+				t.Fatalf("unexpected error received: %s", err)
+			}
+			wrappedInv = invClient.InventoryFactoryFunc(inv)
+			// Validate that the stored objects are now the "localObjs".
+			actualObjs, err := wrappedInv.Load()
+			if err != nil {
+				t.Fatalf("unexpected error received: %s", err)
+			}
+			if !object.SetEquals(tc.localObjs, actualObjs) {
+				t.Errorf("expected objects (%s), got (%s)", tc.localObjs, actualObjs)
+			}
+		})
 	}
 }