-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Change the value parameter of the updateConfiguration API to be required
#10790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.22
Are you sure you want to change the base?
Change the value parameter of the updateConfiguration API to be required
#10790
Conversation
|
@blueorangutan package |
|
@bernardodemarco a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10790 +/- ##
=========================================
Coverage 17.56% 17.56%
- Complexity 15543 15544 +1
=========================================
Files 5909 5909
Lines 529056 529056
Branches 64617 64617
=========================================
+ Hits 92941 92947 +6
+ Misses 425661 425655 -6
Partials 10454 10454
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13217 |
sureshanaparti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm
|
@bernardodemarco , this means that no configuration value can be set to be empty anymore. I can think of some instances an operator would want to do such a think (like a default ldap setting (basedn or so). Is this still possible with your change? |
|
@DaanHoogland, yes, it is possible. This change only encompass scenarios in which the (all-in-one) 🐱 > update configuration name='alert.email.addresses'Currently, the Management Server handles this situation by only returning the configuration details: (all-in-one) 🐱 > update configuration name='alert.email.addresses'
{
"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"
}
}When performing such API calls, the value of the configuration is not changed. To set its value to be empty, the operator needs to explicitly set the (all-in-one) 🐱 > 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": ""
}
}However, I've just realized that, through the UI, it is indeed a little bit awkward for the operator to clear a configuration value. It's required to clear the input field, add an empty space and submit the changes. I'll apply some UI adjustments to enhance UX there |
|
@DaanHoogland, I've just committed this one 01d7c6e Now, operators are able to clean up configuration values (i.e. set its value to
|
|
@DaanHoogland, I’ve identified a possible inconsistency with configurations that accept lists of values (such as those of type CSV). Their default value is an empty string, so when the However, when the I suggest opening a separate issue and PR to address this behavior, as it's not directly related to the scope of this PR. What do you guys think? (cc. @winterhazel, @JoaoJandre, @sureshanaparti) |
eae9d7b to
a66ebe4
Compare
|
@bernardodemarco +1 on separate issue. |
@bernardodemarco addressing it separately seems better for me. |
|
fine by me |
|
Okay, I've just mapped that in a separate issue, vide #10823 |
|
@blueorangutan package |
|
@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13286 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-13235)
|
@bernardodemarco , I think these errors are somewhat related. I restarted the test suite, but can you have a look? |
@DaanHoogland, yes, I also think so... I'll mark it as a draft while I dig into it |
value parameter of the updateConfiguration API to be requiredvalue parameter of the updateConfiguration API to be required
|
@blueorangutan package |
|
@bernardodemarco a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
Outdated
Show resolved
Hide resolved
|
I like this approach better @bernardodemarco , but we still have the same issue; the use of configuration keys is so ubiquitous that we might run into issues somewhere else in the system. I’ll rerun the tests. In the future I think we should introduce a field that contains permissible values, like a pattern or so. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15604 |
@DaanHoogland, the current PR is only proposing to change the Thus, I do not see how we might run into issues somewhere else |
the difference between |
which I will always advise against of course. |
@DaanHoogland, actually, CloudMonkey sets value to be an empty string in both cases:
Thus, both API calls are valid and can be used to clear configuration values |
|
let’s continue @bernardodemarco, my concern is that they work the same and are not distinct (a “” and a <null>). But I don’t see any issue now and we can deal with it later. |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14769)
|
…gerImpl.java Co-authored-by: dahn <daan.hoogland@gmail.com>
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
Outdated
Show resolved
Hide resolved
|
I don’t think this needs more testing but one last regression swoop to be sure. |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 15655 |
|
@DaanHoogland, I've successfully built the current PR here. I'll trigger @blueorangutan's |
|
@blueorangutan package |
|
@bernardodemarco a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15662 |
ah, only the debian build failed and I already checked it would build, but all good anyway. |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
Description
Currently, the
valueparameter of theupdateConfigurationAPI is not required. Thus, when the API is executed and thevalueparameter is not specified, the attributes of the configuration set by thenameparameter are returned.updateConfigurationAPI call without specifying thevalueparameterAs a consequence of that, when managing global configuration through the UI, for instance, when a configuration value is cleared and updated with a blank value, the UI sends the API call without the
valueparameter. Since the UI receives a successful response from the backend, it notifies the user that the setting was successfully updated, which does not actually happens.Additionally, since that the
updateConfigurationAPI is an API to update a resource, it should not be used to list resource details. Currently, however, it is possible to achieve such action through the API.Therefore, aiming to improve UX, avoid possible inconsistent behaviors and maintain a semantic consistency between the API name and its actual behavior, this PR proposes to change the
valueparameter of theupdateConfigurationAPI to be required.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
updateConfigurationAPI through Cloudmonkey, without specifying thevalueparameter, and the following API response was returned:account.cleanup.intervalconfiguration to a blank value and verified that a semantic error message was returned: