Copilot commented on code in PR #13144:
URL: https://github.com/apache/cloudstack/pull/13144#discussion_r3409957755
##########
ui/src/components/view/InstanceVolumesStoragePoolSelectListView.vue:
##########
@@ -170,7 +170,8 @@ export default {
this.volumes = []
getAPI('listVolumes', {
listAll: true,
- virtualmachineid: this.resource.id
+ virtualmachineid: this.resource.id,
+ listsystemvms: true
}).then(response => {
Review Comment:
`listVolumes` is called with `listsystemvms: true` unconditionally. This
component is also used for User VM workflows, and passing `listsystemvms=true`
can cause permission/visibility issues for non-admin users. Make
`listsystemvms` conditional so it’s only enabled for non-UserVm resource types
(SystemVm/VirtualRouter/InternalLbVm/etc.).
##########
ui/src/views/compute/MigrateWizard.vue:
##########
@@ -338,11 +339,11 @@ export default {
submitForm () {
if (this.loading) return
this.loading = true
- const migrateApi = this.isUserVm
- ? this.requiresStorageMigration()
- ? 'migrateVirtualMachineWithVolume'
- : 'migrateVirtualMachine'
- : 'migrateSystemVm'
+ const migrateApi = !this.requiresStorageMigration()
+ ? this.isUserVm
+ ? 'migrateVirtualMachine'
+ : 'migrateSystemVm'
+ : 'migrateVirtualMachineWithVolume'
Review Comment:
The new `migrateApi` selection adds a System VM path that uses
`migrateVirtualMachineWithVolume` when storage migration is required. There are
existing unit tests for `submitForm()` but they don't cover this new branch
(SystemVm + `requiresStorageMigration() === true`), so regressions here may go
unnoticed.
--
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]
