bernardodemarco commented on PR #10790: URL: https://github.com/apache/cloudstack/pull/10790#issuecomment-3424097475
@weizhouapache @DaanHoogland, I've just verified the behavior of the `updateConfiguration` API with the `value` parameter marked as required. I believe the most concerning point regarding backward compatibility (https://github.com/apache/cloudstack/pull/10790#issuecomment-2975481493) would be when updating a configuration value that accepts a list to an empty value. For instance, it is currently possible to execute: ``` update configuration name=alert.email.addresses value="" ``` When a parameter is marked as required, empty string values are not considered by the API layer. Therefore, if the above call is executed with the current PR's changes, the following error is returned: ``` 🙈 Error: (HTTP 431, error code 4350) Invalid value provided for API arg: value ``` Thus, it would be required to specify a blank string, instead of an empty one: ``` (localcloud) 🐈 > update configuration name=alert.email.addresses value=" " { "configuration": { "category": "Alert", "component": "management-server", "description": "Comma separated list of email addresses which are going to receive alert emails.", "displaytext": "Alert email addresses", "group": "Management Server", "isdynamic": false, "name": "alert.email.addresses", "subgroup": "Alerts", "type": "CSV", "value": " " } } ``` --- IMO, this is a valid backward compatibility concern. To workaround it, a possible alternative would be to not mark the `value` parameter as required and to manually check whether it is `null` in the `com.cloud.configuration.ConfigurationManagerImpl#updateConfiguration(org.apache.cloudstack.api.command.admin.config.UpdateCfgCmd)` method. Is this a valid solution in your opinion? -- 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]
