Copilot commented on code in PR #11303:
URL: https://github.com/apache/cloudstack/pull/11303#discussion_r2235024925
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/cluster/UpdateClusterCmd.java:
##########
@@ -60,6 +62,12 @@ public class UpdateClusterCmd extends BaseCmd {
since = "4.20")
private String arch;
+ @Parameter(name = ApiConstants.EXTERNAL_DETAILS,
+ type = CommandType.MAP,
+ description = "Details in key/value pairs to be added to the
extension-resource mapping. Use the format externaldetails[i].<key>=<value>.
Example: externaldetails[0].endpoint.url=https://example.com";,
+ since = "4.21.0")
+ protected Map externalDetails;
Review Comment:
The externalDetails field should be declared with generic types as
`Map<String, String>` to match the return type of getExternalDetails() method
and provide type safety.
```suggestion
protected Map<String, String> externalDetails;
```
##########
ui/src/views/infra/ClusterUpdate.vue:
##########
@@ -159,13 +170,44 @@ export default {
})
this.architectureTypes.opts = typesList
},
+ fetchExtensionResourceMapDetails () {
+ this.form.externaldetails = null
+ if (!this.resource.id || !this.resource.extensionid) {
+ return
+ }
+ this.loading = true
+ const params = {
+ id: this.resource.extensionid,
+ details: 'resource'
+ }
+ getAPI('listExtensions', params).then(json => {
+ this.extension = json.listextensionsresponse.extension[0]
+ if (!this.extension?.resources) {
+ return null
+ }
+ const resourceMap = this.extension.resources.find(r => r.id ===
this.resource.id)
+ if (!resourceMap || !resourceMap.details || typeof resourceMap.details
!== 'object') {
+ this.form.externaldetails = null
+ }
+ this.form.externaldetails = resourceMap.details
Review Comment:
The condition sets form.externaldetails to null but doesn't return early,
causing the next line to potentially overwrite it. Add a return statement or
use else if to prevent the assignment on line 192.
```suggestion
} else {
this.form.externaldetails = resourceMap.details
}
```
##########
ui/src/views/infra/ClusterUpdate.vue:
##########
@@ -159,13 +170,44 @@ export default {
})
this.architectureTypes.opts = typesList
},
+ fetchExtensionResourceMapDetails () {
+ this.form.externaldetails = null
+ if (!this.resource.id || !this.resource.extensionid) {
+ return
+ }
+ this.loading = true
+ const params = {
+ id: this.resource.extensionid,
+ details: 'resource'
+ }
+ getAPI('listExtensions', params).then(json => {
+ this.extension = json.listextensionsresponse.extension[0]
+ if (!this.extension?.resources) {
+ return null
+ }
+ const resourceMap = this.extension.resources.find(r => r.id ===
this.resource.id)
+ if (!resourceMap || !resourceMap.details || typeof resourceMap.details
!== 'object') {
+ this.form.externaldetails = null
+ }
+ this.form.externaldetails = resourceMap.details
+ }).catch(error => {
+ this.$notifyError(error)
+ }).finally(() => {
+ this.loading = false
+ })
+ },
handleSubmit () {
this.formRef.value.validate().then(() => {
const values = toRaw(this.form)
console.log(values)
Review Comment:
Console.log statement should be removed from production code as it's used
for debugging purposes.
```suggestion
```
##########
framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java:
##########
@@ -1478,6 +1478,45 @@ public String
handleExtensionServerCommands(ExtensionServerActionBaseCommand com
return GsonHelper.getGson().toJson(answers);
}
+ @Override
+ public Pair<Boolean, ExtensionResourceMap>
extensionResourceMapDetailsNeedUpdate(long resourceId,
+ ExtensionResourceMap.ResourceType resourceType,
Map<String, String> externalDetails) {
+ if (MapUtils.isEmpty(externalDetails)) {
+ return new Pair<>(false, null);
+ }
+ ExtensionResourceMapVO extensionResourceMapVO =
+ extensionResourceMapDao.findByResourceIdAndType(resourceId,
resourceType);
+ if (extensionResourceMapVO == null) {
+ return new Pair<>(true, null);
+ }
+ Map<String, String> mapDetails =
+
extensionResourceMapDetailsDao.listDetailsKeyPairs(extensionResourceMapVO.getId());
+ if (MapUtils.isEmpty(mapDetails) || mapDetails.size() !=
externalDetails.size()) {
+ return new Pair<>(true, extensionResourceMapVO);
+ }
+ for (Map.Entry<String, String> entry : externalDetails.entrySet()) {
+ String key = entry.getKey();
+ String value = entry.getValue();
+ if (!value.equals(mapDetails.get(key))) {
Review Comment:
The size comparison logic is incorrect. If mapDetails is empty but
externalDetails has values, or if externalDetails is smaller than mapDetails,
the method should still return true to indicate an update is needed. The
current logic only checks if sizes are different, not if an update is actually
required.
```suggestion
if (MapUtils.isEmpty(mapDetails)) {
// If mapDetails is empty but externalDetails has values, an
update is needed
return new Pair<>(true, extensionResourceMapVO);
}
if (mapDetails.size() != externalDetails.size()) {
// If the sizes are different, an update is needed
return new Pair<>(true, extensionResourceMapVO);
}
for (Map.Entry<String, String> entry : externalDetails.entrySet()) {
String key = entry.getKey();
String value = entry.getValue();
if (!mapDetails.containsKey(key) ||
!value.equals(mapDetails.get(key))) {
// If a key is missing or a value does not match, an update
is needed
```
--
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: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
