GabrielBrascher commented on a change in pull request #4548:
URL: https://github.com/apache/cloudstack/pull/4548#discussion_r544291460
##########
File path:
server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java
##########
@@ -368,6 +358,12 @@ public void importUnmanagedInstanceTest() {
when(importUnmanageInstanceCmd.getName()).thenReturn("TestInstance");
when(importUnmanageInstanceCmd.getAccountName()).thenReturn(null);
when(importUnmanageInstanceCmd.getDomainId()).thenReturn(null);
+ Map<String,Long> diskOfferings = new HashMap();
+ getDisks().forEach((disk) -> {
+ diskOfferings.put(disk.getDiskId(), 43L);
+ });
+ diskOfferings.remove("WrongRootDisk");
+
when(importUnmanageInstanceCmd.getDataDiskToDiskOfferingList()).thenReturn(diskOfferings);
Review comment:
I don't see how this test method is validating their PR.
I was expecting some sort of assertion or validation on the lists returned
to ensure that the implemented method is indeed returning the correct order?
##########
File path:
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java
##########
@@ -1248,6 +1261,25 @@ public UserVmResponse
importUnmanagedInstance(ImportUnmanagedInstanceCmd cmd) {
return cmdList;
}
+ /**
+ * VCenter send a list of disk in lexicographical order, but not in every
+ * case is hard disk one the root disk. Therefore we must check if hd one
+ * the root disk and if not we search it and add it to the top of the
+ * list
Review comment:
I see some points to improve here and took the liberty to propose the
following Javadoc block:
```
VCenter sends a list of disks in lexicographical order, but not in every
case the first volume on the list is the root volume. Therefore this method
checks if the first element of the list is the root disk; if not, it searches
it and adds it to the top of the list
```
Disclaimer 1: I changed from hard disk to "volume" due to the term hard
disks being related to the technology/device used to store volumes. In many DCs
Hard Disks are being mixed with (or replaced by) Solid State Disks.
Disclaimer 2: as far as I know, this method is not used only in VCenter
cases. We have some situations on CloudStack where methods are used only by a
hypervisor/storage technology, but I think that this is not the case.
Therefore, maybe the documentation should be enhanced to avoid
misunderstandings on that matter.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
