Copilot commented on code in PR #13203:
URL: https://github.com/apache/cloudstack/pull/13203#discussion_r3279470565
##########
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java:
##########
@@ -906,7 +907,9 @@ public Pair<Boolean, String>
restoreVMToDifferentLocation(String restorePointId,
if (restoreLocation == null) {
restoreLocation = RESTORE_VM_SUFFIX + UUID.randomUUID().toString();
}
- final String datastoreId = dataStoreUuid.replace("-","");
+ final String datastoreId = UuidUtils.isUuid(dataStoreUuid) ?
+ dataStoreUuid.replace("-","") :
+ dataStoreUuid;
Review Comment:
The new UUID/name handling for `dataStoreUuid` (UUIDs are normalized by
stripping hyphens, non-UUID strings are passed through) is not covered by unit
tests. Please add a test case to ensure datastore names containing dashes are
not modified, while UUID inputs are still normalized as expected.
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -1496,6 +1497,20 @@ public boolean restoreBackupVolumeAndAttachToVM(final
String backedUpVolumeUuid,
return true;
}
+ /**
+ * For VMFS datastores, the identifier to be used for Veeam restore is the
datastore name.
+ * Otherwise, possible values are the datastore UUID and the datastore
name..
Review Comment:
Typo in the Javadoc: the sentence ends with a double period ("name..").
Please fix the punctuation to avoid introducing new doc typos.
##########
server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java:
##########
@@ -2242,4 +2242,33 @@ public void testRestoreBackupVolumeMismatch() {
verify(vmInstanceDao, times(1)).findByIdIncludingRemoved(vmId);
verify(volumeDao, times(1)).findByInstance(vmId);
}
+
+ @Test
+ public void testGetDatastorePossibleValuesNFS() {
+ String poolName = "NFS-Pool-Name";
+ String poolUuid = UUID.randomUUID().toString();
+
+ StoragePoolVO pool = Mockito.mock(StoragePoolVO.class);
+
when(pool.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem);
+ when(pool.getName()).thenReturn(poolName);
+ when(pool.getUuid()).thenReturn(poolUuid);
+
+ String[] datastorePossibleValues =
backupManager.getDatastorePossibleValues(pool);
+ Assert.assertEquals(2, datastorePossibleValues.length);
+ Assert.assertEquals(poolUuid, datastorePossibleValues[0]);
+ Assert.assertEquals(poolName, datastorePossibleValues[1]);
+ }
+
+ @Test
+ public void testGetDatastorePossibleValuesVSAN() {
+ String poolName = "VSAN-Pool-Name";
+
+ StoragePoolVO pool = Mockito.mock(StoragePoolVO.class);
+ when(pool.getPoolType()).thenReturn(Storage.StoragePoolType.VMFS);
+ when(pool.getName()).thenReturn(poolName);
Review Comment:
The test name `testGetDatastorePossibleValuesVSAN` asserts behavior based on
`StoragePoolType.VMFS`, not something vSAN-specific. Renaming it to reflect
VMFS would make the test intent clearer and avoid confusion about what storage
types are actually covered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
