From 67dd3d77d1ad8bc35a28bee5bc7c5513312d1d96 Mon Sep 17 00:00:00 2001
From: Ricardo Katz <rkatz@vmware.com>
Date: Mon, 30 Oct 2023 10:26:39 -0300
Subject: [PATCH] Fix comments on spbm review

---
 pkg/services/govmomi/service.go      | 20 ++++++++++----------
 pkg/services/govmomi/service_test.go |  1 +
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/pkg/services/govmomi/service.go b/pkg/services/govmomi/service.go
index a6bd7e6e10..4bc797c299 100644
--- a/pkg/services/govmomi/service.go
+++ b/pkg/services/govmomi/service.go
@@ -401,16 +401,16 @@ func (vms *VMService) reconcileStoragePolicy(ctx context.Context, virtualMachine
 		return err
 	}
 
-	arrayOfEntities := make([]pbmTypes.PbmServerObjectRef, 0)
+	disksRefs := make([]pbmTypes.PbmServerObjectRef, 0)
 	// diskMap is just an auxiliar map so we don't need to iterate over and over disks to get their configs
 	// if we realize they are not on the right storage policy
 	diskMap := make(map[string]*types.VirtualDisk)
 
 	disks := devices.SelectByType((*types.VirtualDisk)(nil))
 
-	// We iterate over disks and create an array of entities, so we just need to make a single call
+	// We iterate over disks and create an array of disks refs, so we just need to make a single call
 	// against vCenter, instead of one call per disk
-	// the diskMap is an auxiliar way of, besides the arrayOfEntities, we have a "searchable" disk configuration
+	// the diskMap is an auxiliar way of, besides the disksRefs, we have a "searchable" disk configuration
 	// in case we need to reconfigure a disk, to get its config
 	for _, d := range disks {
 		disk := d.(*types.VirtualDisk)
@@ -418,23 +418,23 @@ func (vms *VMService) reconcileStoragePolicy(ctx context.Context, virtualMachine
 		diskID := fmt.Sprintf("%s:%d", virtualMachineCtx.Obj.Reference().Value, disk.Key)
 		diskMap[diskID] = disk
 
-		arrayOfEntities = append(arrayOfEntities, pbmTypes.PbmServerObjectRef{
+		disksRefs = append(disksRefs, pbmTypes.PbmServerObjectRef{
 			ObjectType: string(pbmTypes.PbmObjectTypeVirtualDiskId),
 			Key:        diskID,
 		})
 	}
 
-	entities, err := QueryAssociatedProfiles(ctx, pbmClient, arrayOfEntities)
+	diskObjects, err := QueryAssociatedProfiles(ctx, pbmClient, disksRefs)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "unable to query disks associated profiles")
 	}
 
-	for k := range entities {
-		if !isStoragePolicyIDPresent(storageProfileID, entities[k]) {
-			virtualMachineCtx.Logger.V(5).Info("storage policy not found on disk, adding for reconciliation", "disk", entities[k].Object.Key)
+	for k := range diskObjects {
+		if !isStoragePolicyIDPresent(storageProfileID, diskObjects[k]) {
+			virtualMachineCtx.Logger.V(5).Info("storage policy not found on disk, adding for reconciliation", "disk", diskObjects[k].Object.Key)
 			config := &types.VirtualDeviceConfigSpec{
 				Operation: types.VirtualDeviceConfigSpecOperationEdit,
-				Device:    diskMap[entities[k].Object.Key],
+				Device:    diskMap[diskObjects[k].Object.Key],
 				Profile: []types.BaseVirtualMachineProfileSpec{
 					&types.VirtualMachineDefinedProfileSpec{ProfileId: storageProfileID},
 				},
diff --git a/pkg/services/govmomi/service_test.go b/pkg/services/govmomi/service_test.go
index 8a3d7d57a5..a0867e3823 100644
--- a/pkg/services/govmomi/service_test.go
+++ b/pkg/services/govmomi/service_test.go
@@ -168,6 +168,7 @@ func Test_ReconcileStoragePolicy(t *testing.T) {
 	})
 
 	t.Run("when the requested storage policy exists should pass", func(t *testing.T) {
+		// This Method should be implemented on Govmomi vcsim and then we can unskip this test
 		t.Skip("PbmQueryAssociatedProfiles is not yet implemented on PBM simulator")
 		g = NewWithT(t)
 		before()