Copilot commented on code in PR #11974:
URL: https://github.com/apache/cloudstack/pull/11974#discussion_r2735202353
##########
ui/src/views/compute/EditVM.vue:
##########
@@ -425,6 +425,8 @@ export default {
}
if (values.extraconfig && values.extraconfig.length > 0) {
params.extraconfig = encodeURIComponent(values.extraconfig)
Review Comment:
In the UI, whitespace-only values for `extraconfig` are treated as “present”
(length > 0), so the request sends `extraconfig` instead of
`cleanupextraconfig`. On the server side `StringUtils.isNotBlank` treats
whitespace as blank, so this results in no update and the existing extraconfig
is not cleaned. Consider trimming before the length check (or using a blank
check) so clearing via whitespace triggers cleanup as expected.
```suggestion
const extraConfig = typeof values.extraconfig === 'string' ?
values.extraconfig.trim() : values.extraconfig
if (extraConfig && extraConfig.length > 0) {
params.extraconfig = encodeURIComponent(extraConfig)
```
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -2982,14 +2999,7 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd)
throws ResourceUnavailableEx
vmInstance.setDetails(details);
_vmDao.saveDetails(vmInstance);
}
- if (StringUtils.isNotBlank(extraConfig)) {
- if (EnableAdditionalVmConfig.valueIn(accountId)) {
- logger.info("Adding extra configuration to user vm: " +
vmInstance.getUuid());
- addExtraConfig(vmInstance, extraConfig);
- } else {
- throw new InvalidParameterValueException("attempted
setting extraconfig but enable.additional.vm.configuration is disabled");
- }
- }
+ updateVmExtraConfig(userVm, extraConfig, cleanupExtraConfig);
}
Review Comment:
`cleanupextraconfig` is only applied in the `else` branch when
`cleanupDetails` is false. If a caller submits both `cleanupdetails=true` and
`cleanupextraconfig=true`, extraconfig will not be removed (since the
cleanupDetails path explicitly skips extra config). Consider handling
`cleanupExtraConfig` independently of `cleanupDetails` so the new API flag is
never silently ignored.
```suggestion
}
updateVmExtraConfig(userVm, extraConfig, cleanupExtraConfig);
```
##########
engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDetailsDaoImpl.java:
##########
@@ -31,4 +34,18 @@ public void addDetail(long resourceId, String key, String
value, boolean display
super.addDetail(new VMInstanceDetailVO(resourceId, key, value,
display));
}
+ @Override
+ public int removeDetailsWithPrefix(long vmId, String prefix) {
+ if (StringUtils.isBlank(prefix)) {
+ return 0;
+ }
+ SearchBuilder<VMInstanceDetailVO> sb = createSearchBuilder();
+ sb.and("vmId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
+ sb.and("prefix", sb.entity().getName(), SearchCriteria.Op.LIKE);
+ sb.done();
+ SearchCriteria<VMInstanceDetailVO> sc = sb.create();
+ sc.setParameters("vmId", vmId);
+ sc.setParameters("prefix", prefix + "%");
+ return super.remove(sc);
Review Comment:
`removeDetailsWithPrefix` calls `super.remove(sc)`, which bypasses method
dispatch. This makes the new Mockito spy-based unit tests unable to stub/verify
the removal call (and will likely execute the real DAO removal logic during
unit tests). Prefer calling `remove(sc)` instead so the method can be
mocked/spied and behaves consistently with other methods in
`ResourceDetailsDaoBase` (which call `remove(sc)` directly).
```suggestion
return remove(sc);
```
--
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]
