Copilot commented on code in PR #13235:
URL: https://github.com/apache/cloudstack/pull/13235#discussion_r3304272336
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -1816,6 +1817,45 @@ protected void checkConversionStoragePool(Long
convertStoragePoolId, boolean for
}
}
+ protected Long resolveImplicitConversionStoragePoolId(Cluster
destinationCluster, Long convertStoragePoolId,
+ boolean
forceConvertToPool) {
+ if (!forceConvertToPool || convertStoragePoolId != null) {
+ return convertStoragePoolId;
+ }
+
+ List<StoragePoolVO> candidatePools =
getImplicitForceConvertToPoolStoragePools(destinationCluster);
+ if (candidatePools.isEmpty()) {
+ logFailureAndThrowException(String.format("The parameter
forceconverttopool is set to true, but no suitable primary storage pool was
found in cluster %s",
+ destinationCluster.getName()));
+ }
+ if (candidatePools.size() > 1) {
+ logFailureAndThrowException(String.format("The parameter
forceconverttopool is set to true, but multiple suitable primary storage pools
were found in cluster %s. Please provide convertinstancepoolid.",
+ destinationCluster.getName()));
Review Comment:
The exception messages here say the suitable pool(s) were/weren’t found "in
cluster %s", but `getImplicitForceConvertToPoolStoragePools` searches both
cluster-wide and zone-wide primary storage pools. This can mislead operators
and contradict the documented behavior; please update the messages to
explicitly mention cluster-or-zone scope (and ideally distinguish which
scope(s) matched).
##########
ui/src/views/tools/ImportUnmanagedInstance.vue:
##########
@@ -1186,6 +1197,14 @@ export default {
this.fetchKvmHostsForConversion()
}
this.resetStorageOptionsForConversion()
+ if (val) {
+ this.selectPrimaryStorageForForcedConversion()
+ }
+ },
+ selectPrimaryStorageForForcedConversion () {
+ this.selectedStorageOptionForConversion = 'primary'
+ this.showStoragePoolsForConversion = true
+ this.fetchStoragePoolsForConversion()
Review Comment:
`selectPrimaryStorageForForcedConversion()` sets
`selectedStorageOptionForConversion` programmatically and immediately fetches
pools, but the storage-option UI uses `CheckBoxSelectPair`, which does not
react to parent state and won’t preselect an option unless `defaultSelectValue`
is provided. This can leave the storage option dropdown visually unselected
while pools are shown/auto-selected, and if the user selects a conversion host
afterwards the pool list may be based on the wrong scope until manually
refreshed. Consider wiring a defaultSelectValue of 'primary' when forced
conversion is enabled and refreshing pools when the conversion host changes
while 'primary' is implicitly selected.
--
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]
