Change the `value` parameter of the `updateConfiguration` API to be required

[bernardodemarco]

Description

Currently, the value parameter of the updateConfiguration API is not required. Thus, when the API is executed and the value parameter is not specified, the attributes of the configuration set by the name parameter are returned.

updateConfiguration API call without specifying the value parameter

> update configuration name="quota.enable.service"
{
  "configuration": {
    "category": "Advanced",
    "component": "QUOTA-PLUGIN",
    "defaultvalue": "false",
    "description": "Indicates whether Quota plugin is enabled or not.",
    "displaytext": "Quota enable service",
    "group": "Miscellaneous",
    "isdynamic": true,
    "name": "quota.enable.service",
    "subgroup": "Quota",
    "type": "Boolean"
  }
}

As 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 value parameter. 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 updateConfiguration API 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 value parameter of the updateConfiguration API to be required.

Types of changes

• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Bug fix (non-breaking change which fixes an issue)
• [X] Enhancement (improves an existing feature and functionality)
• [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
• [ ] build/CI
• [ ] test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• [ ] Major
• [X] Minor

Screenshots (if appropriate):

How Has This Been Tested?

• Executed the updateConfiguration API through Cloudmonkey, without specifying the value parameter, and the following API response was returned:

> update configuration name="quota.enable.service"
Missing required parameters:  value

• Accessed the global settings page, updated the account.cleanup.interval configuration to a blank value and verified that a semantic error message was returned:

[bernardodemarco]

@blueorangutan package

[blueorangutan]

@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[bot]]

Codecov Report

:x: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 17.56%. Comparing base (3d6cafe) to head (fb158ea). :warning: Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
.../cloud/configuration/ConfigurationManagerImpl.java	0.00%	2 Missing :warning:

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           

Flag	Coverage Δ
uitests	3.58% <ø> (ø)
unittests	18.63% <0.00%> (+<0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13217

[DaanHoogland]

@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?

[bernardodemarco]

@DaanHoogland, yes, it is possible. This change only encompass scenarios in which the value parameter is not included in the API request. For instance:

(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 value parameter to an empty string:

(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

[bernardodemarco]

@DaanHoogland, I've just committed this one 01d7c6e1e8bca0c0de4ce0f7186c9315575a54ff

Now, operators are able to clean up configuration values (i.e. set its value to NULL on the DB) for the configurations with type equal to CSV, Order, WhitespaceSeparatedListWithOptions and String.

• String configuration type:

  https://github.com/user-attachments/assets/b327b43d-c369-4563-a357-5e8c51c5d47c

• Order configuration type:

  https://github.com/user-attachments/assets/a23c11df-272b-4f3d-8c7c-f229ebb65bb2

• CSV configuration type:

  https://github.com/user-attachments/assets/dd7b1b33-9b86-4268-8718-bf2b1c14b6fb

• WhitespaceSeparatedListWithOptions configuration type:

  https://github.com/user-attachments/assets/a28c209f-90e8-42d3-846b-ef66c439ff7d

[bernardodemarco]

@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 resetConfiguration API is called, the value is reset to an empty string, as expected.

However, when the updateConfiguration API is used with the value parameter set to an empty string, the Management Server currently stores NULL for the configuration in the database. I believe this could lead to bugs down the line, such as potential NPEs.

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)

[JoaoJandre]

@bernardodemarco +1 on separate issue.

[winterhazel]

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)

@bernardodemarco addressing it separately seems better for me.

[DaanHoogland]

fine by me

[bernardodemarco]

Okay, I've just mapped that in a separate issue, vide #10823

[JoaoJandre]

@blueorangutan package

[blueorangutan]

@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.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13286

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-13235) Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8 Total time taken: 52210 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10790-t13235-kvm-ol8.zip Smoke tests completed. 138 look OK, 3 have errors, 0 did not run Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_UpdateConfigParamWithScope	Error	0.09	test_global_settings.py
test_01_deploy_vm_with_extraconfig_throws_exception_kvm	Error	0.07	test_deploy_vm_extra_config_data.py
test_02_deploy_vm_with_extraconfig_kvm	Error	69.54	test_deploy_vm_extra_config_data.py
test_03_update_vm_with_extraconfig_kvm	Error	143.61	test_deploy_vm_extra_config_data.py
test_01_migrate_vm_strict_tags_success	Error	0.07	test_vm_strict_host_tags.py
test_02_migrate_vm_strict_tags_failure	Error	0.06	test_vm_strict_host_tags.py
ContextSuite context=TestMigrateVMStrictTags>:teardown	Error	0.13	test_vm_strict_host_tags.py
test_01_restore_vm_strict_tags_success	Error	0.05	test_vm_strict_host_tags.py
test_02_restore_vm_strict_tags_failure	Error	0.05	test_vm_strict_host_tags.py
ContextSuite context=TestRestoreVMStrictTags>:teardown	Error	0.09	test_vm_strict_host_tags.py
test_01_scale_vm_strict_tags_success	Error	0.06	test_vm_strict_host_tags.py
test_02_scale_vm_strict_tags_failure	Error	0.06	test_vm_strict_host_tags.py
ContextSuite context=TestScaleVMStrictTags>:teardown	Error	0.10	test_vm_strict_host_tags.py
test_01_deploy_vm_on_specific_host_without_strict_tags	Error	0.06	test_vm_strict_host_tags.py
test_02_deploy_vm_on_any_host_without_strict_tags	Error	0.07	test_vm_strict_host_tags.py
test_03_deploy_vm_on_specific_host_with_strict_tags_success	Error	0.06	test_vm_strict_host_tags.py
test_04_deploy_vm_on_any_host_with_strict_tags_success	Error	0.05	test_vm_strict_host_tags.py
test_05_deploy_vm_on_specific_host_with_strict_tags_failure	Error	0.05	test_vm_strict_host_tags.py
test_06_deploy_vm_on_any_host_with_strict_tags_failure	Error	0.05	test_vm_strict_host_tags.py
ContextSuite context=TestVMDeploymentPlannerStrictTags>:teardown	Error	0.10	test_vm_strict_host_tags.py

[DaanHoogland]

[SF] Trillian test result (tid-13235) Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8 Total time taken: 52210 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10790-t13235-kvm-ol8.zip Smoke tests completed. 138 look OK, 3 have errors, 0 did not run Only failed and skipped tests results shown below:

Test Result Time (s) Test File test_UpdateConfigParamWithScope Error 0.09 test_global_settings.py test_01_deploy_vm_with_extraconfig_throws_exception_kvm Error 0.07 test_deploy_vm_extra_config_data.py test_02_deploy_vm_with_extraconfig_kvm Error 69.54 test_deploy_vm_extra_config_data.py test_03_update_vm_with_extraconfig_kvm Error 143.61 test_deploy_vm_extra_config_data.py test_01_migrate_vm_strict_tags_success Error 0.07 test_vm_strict_host_tags.py test_02_migrate_vm_strict_tags_failure Error 0.06 test_vm_strict_host_tags.py ContextSuite context=TestMigrateVMStrictTags>:teardown Error 0.13 test_vm_strict_host_tags.py test_01_restore_vm_strict_tags_success Error 0.05 test_vm_strict_host_tags.py test_02_restore_vm_strict_tags_failure Error 0.05 test_vm_strict_host_tags.py ContextSuite context=TestRestoreVMStrictTags>:teardown Error 0.09 test_vm_strict_host_tags.py test_01_scale_vm_strict_tags_success Error 0.06 test_vm_strict_host_tags.py test_02_scale_vm_strict_tags_failure Error 0.06 test_vm_strict_host_tags.py ContextSuite context=TestScaleVMStrictTags>:teardown Error 0.10 test_vm_strict_host_tags.py test_01_deploy_vm_on_specific_host_without_strict_tags Error 0.06 test_vm_strict_host_tags.py test_02_deploy_vm_on_any_host_without_strict_tags Error 0.07 test_vm_strict_host_tags.py test_03_deploy_vm_on_specific_host_with_strict_tags_success Error 0.06 test_vm_strict_host_tags.py test_04_deploy_vm_on_any_host_with_strict_tags_success Error 0.05 test_vm_strict_host_tags.py test_05_deploy_vm_on_specific_host_with_strict_tags_failure Error 0.05 test_vm_strict_host_tags.py test_06_deploy_vm_on_any_host_with_strict_tags_failure Error 0.05 test_vm_strict_host_tags.py ContextSuite context=TestVMDeploymentPlannerStrictTags>:teardown Error 0.10 test_vm_strict_host_tags.py

@bernardodemarco , I think these errors are somewhat related. I restarted the test suite, but can you have a look?

[bernardodemarco]

@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

[blueorangutan]

[SF] Trillian test result (tid-13245) Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8 Total time taken: 61016 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10790-t13245-kvm-ol8.zip Smoke tests completed. 137 look OK, 4 have errors, 0 did not run Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_UpdateConfigParamWithScope	Error	0.14	test_global_settings.py
test_01_deploy_vm_with_extraconfig_throws_exception_kvm	Error	0.08	test_deploy_vm_extra_config_data.py
test_02_deploy_vm_with_extraconfig_kvm	Error	72.06	test_deploy_vm_extra_config_data.py
test_03_update_vm_with_extraconfig_kvm	Error	148.08	test_deploy_vm_extra_config_data.py
test_03_register_template	Error	1.18	test_resource_names.py
test_04_register_iso	Error	525.71	test_resource_names.py
test_01_migrate_vm_strict_tags_success	Error	0.06	test_vm_strict_host_tags.py
test_02_migrate_vm_strict_tags_failure	Error	0.06	test_vm_strict_host_tags.py
ContextSuite context=TestMigrateVMStrictTags>:teardown	Error	0.14	test_vm_strict_host_tags.py
test_01_restore_vm_strict_tags_success	Error	0.06	test_vm_strict_host_tags.py
test_02_restore_vm_strict_tags_failure	Error	0.05	test_vm_strict_host_tags.py
ContextSuite context=TestRestoreVMStrictTags>:teardown	Error	0.12	test_vm_strict_host_tags.py
test_01_scale_vm_strict_tags_success	Error	0.06	test_vm_strict_host_tags.py
test_02_scale_vm_strict_tags_failure	Error	0.07	test_vm_strict_host_tags.py
ContextSuite context=TestScaleVMStrictTags>:teardown	Error	0.13	test_vm_strict_host_tags.py
test_01_deploy_vm_on_specific_host_without_strict_tags	Error	0.07	test_vm_strict_host_tags.py
test_02_deploy_vm_on_any_host_without_strict_tags	Error	0.07	test_vm_strict_host_tags.py
test_03_deploy_vm_on_specific_host_with_strict_tags_success	Error	0.06	test_vm_strict_host_tags.py
test_04_deploy_vm_on_any_host_with_strict_tags_success	Error	0.06	test_vm_strict_host_tags.py
test_05_deploy_vm_on_specific_host_with_strict_tags_failure	Error	0.06	test_vm_strict_host_tags.py
test_06_deploy_vm_on_any_host_with_strict_tags_failure	Error	0.06	test_vm_strict_host_tags.py
ContextSuite context=TestVMDeploymentPlannerStrictTags>:teardown	Error	0.13	test_vm_strict_host_tags.py

[github-actions[bot]]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[bernardodemarco]

@DaanHoogland @sureshanaparti can we run the CI here? I believe that the majority of the integration tests (if not all of them) should be fixed now

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@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.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13764

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-13516) Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8 Total time taken: 55458 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10790-t13516-kvm-ol8.zip Smoke tests completed. 141 look OK, 0 have errors, 0 did not run Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[DaanHoogland]

@bernardodemarco what is the status on this?