Description of problem: When using REST to access advanced settings, symbols become strings, openshift: refresh_interval: "15.minutes" inventory_object_refresh: true inventory_collections : saver_strategy: "batch" get_container_images: true store_unused_images: true Should be: openshift: refresh_interval: "15.minutes" inventory_object_refresh: true inventory_collections : saver_strategy: :batch get_container_images: true store_unused_images: true Alternatively, and possibly better, is to make the OpenShift provider accept the string and not use symbols in the yamls at all. Version-Release number of selected component (if applicable): 5.9.0.1 How reproducible: 100% Steps to Reproduce: 1. Access the endpoint api/servers/1/settings 2. 3. Actual results: saver_strategy: "batch" Expected results: saver_strategy: :batch Additional info:
I am going fix this specific case for refresh, by accepting strings
https://github.com/ManageIQ/manageiq/pull/17168
When is that saver_strategy setting getting changed to symbols ? is that when updated from the API (PATCH /api/servers/1/settings) or some other way ? Thanks.
On GET, you can do it from within the browser, just access the API and you will see it.
Ok, still confused here. Question was when it was changing to string. Here's what I've noticed: $ bundle exec rails c > Settings.ems_refresh.openshift.inventory_collections.saver_strategy :batch Updating it via REST API as follows: PATCH /api/servers/1/settings { "ems_refresh" : { "openshift" : { "inventory_collections" : { "saver_strategy" : "batch" } } } } Then verifying value again as follows: > Settings.reload! > Settings.ems_refresh.openshift.inventory_collections.saver_strategy "batch" I just would like verification that an API PATCH like the above is causing the issue you are having. Thanks.
Putting back to API team, based on conversation in https://github.com/ManageIQ/manageiq/pull/17168
Looks like it is feasible to change all places to use strings: https://github.com/ManageIQ/manageiq/issues/17201
I agree with Beni that we wshould just switch all values-as-symbols to Strings. Additionally, we have to deal with upgrades gracefully so I propose the following. 1. Create a data migration that fixes any already-changed values back to Strings. 2. Update all callers to handle both Strings and Symbols (Handling both allows this bit to be backportable without needing to backport a migration) 3. Add a config validator that no symbols are added as values. 4. Handle string keys in methods for updating Settings (e.g. add_settings_for_resource), so that incoming string-keys via the API are supported. Ladas, I've reopened your PR, which handles one of the cases for 2, and I'm looking at the rest as described in https://github.com/ManageIQ/manageiq/issues/17201
https://github.com/ManageIQ/manageiq/pull/17238
New commit detected on ManageIQ/manageiq/master: https://github.com/ManageIQ/manageiq/commit/1b8cd76c7b8ff0ec5bf734a70d3e0ae02acfe974 commit 1b8cd76c7b8ff0ec5bf734a70d3e0ae02acfe974 Author: Jason Frey <jfrey> AuthorDate: Thu Mar 29 18:02:57 2018 -0400 Commit: Jason Frey <jfrey> CommitDate: Thu Mar 29 18:02:57 2018 -0400 Add specs to show save! supports string keys https://bugzilla.redhat.com/show_bug.cgi?id=1558031 spec/lib/vmdb/settings_spec.rb | 22 + 1 file changed, 22 insertions(+)
4 seems to be already handled, as I've written specs to support string keys and they all passed outright. Separately, we also found that the error message "configuration invalid" is just useless. This BZ probably would not have been opened if a) we gave a better error message detailing why the configuration is invalid b) we didn't return status code 500 but instead returned 400 Bad Request or something more specific.
https://github.com/ManageIQ/manageiq/pull/17247
New commit detected on ManageIQ/manageiq/master: https://github.com/ManageIQ/manageiq/commit/2d0451184f95bb7796243efb08238b3ef651a516 commit 2d0451184f95bb7796243efb08238b3ef651a516 Author: Ladislav Smola <lsmola> AuthorDate: Mon Mar 19 10:10:29 2018 -0400 Commit: Ladislav Smola <lsmola> CommitDate: Mon Mar 19 10:10:29 2018 -0400 Accept non symbol values from settings Accept non symbol values from settings. With recent API change, we are getting symbols transformed to strings. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1558031 app/models/manager_refresh/inventory_collection.rb | 2 + app/models/manager_refresh/save_inventory.rb | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)
New commit detected on ManageIQ/manageiq/master: https://github.com/ManageIQ/manageiq/commit/e089d49260a38617c97ea3c96c4aebd17756895c commit e089d49260a38617c97ea3c96c4aebd17756895c Author: Jason Frey <jfrey> AuthorDate: Tue Apr 3 15:17:34 2018 -0400 Commit: Jason Frey <jfrey> CommitDate: Tue Apr 3 15:17:34 2018 -0400 Give more information on a failed configuration validation. https://bugzilla.redhat.com/show_bug.cgi?id=1558031 app/models/authenticator.rb | 2 +- app/models/authenticator/ldap.rb | 2 +- lib/vmdb/config/validator.rb | 2 +- lib/vmdb/settings.rb | 21 +- spec/lib/vmdb/settings_spec.rb | 2 +- 5 files changed, 21 insertions(+), 8 deletions(-)
New commit detected on ManageIQ/manageiq-providers-kubernetes/master: https://github.com/ManageIQ/manageiq-providers-kubernetes/commit/673b088b2215b170cfef4d9dfc1e947670a05e28 commit 673b088b2215b170cfef4d9dfc1e947670a05e28 Author: Jason Frey <fryguy9> AuthorDate: Mon Apr 9 15:02:32 2018 -0400 Commit: Jason Frey <fryguy9> CommitDate: Mon Apr 9 15:02:32 2018 -0400 Change saver_strategy value to String Symbols in configuration values are problematic because Symbols do not roundtrip through JSON. Since the API now exposes Settings, it's not possible to set Symbols as values. The saver_strategy was fixed in https://github.com/ManageIQ/manageiq/pull/17168 to support String values, and the next step is to remove all Symbols from the Settings. https://github.com/ManageIQ/manageiq/issues/17201 https://bugzilla.redhat.com/show_bug.cgi?id=1558031 app/models/manageiq/providers/kubernetes/container_manager/inventory_collections.rb | 2 +- config/settings.yml | 2 +- spec/models/manageiq/providers/kubernetes/container_manager/refresher_spec.rb | 6 +- spec/models/manageiq/providers/kubernetes/container_manager/targeted_refresh/targeted_refresh_spec.rb | 6 +- 4 files changed, 8 insertions(+), 8 deletions(-)
New commit detected on ManageIQ/manageiq-providers-azure/master: https://github.com/ManageIQ/manageiq-providers-azure/commit/2b2a2504566f7e84964534ee05f0ec1e09b00b0b commit 2b2a2504566f7e84964534ee05f0ec1e09b00b0b Author: Jason Frey <fryguy9> AuthorDate: Mon Apr 9 12:07:14 2018 -0400 Commit: Jason Frey <fryguy9> CommitDate: Mon Apr 9 12:07:14 2018 -0400 Change saver_strategy value to String Symbols in configuration values are problematic because Symbols do not roundtrip through JSON. Since the API now exposes Settings, it's not possible to set Symbols as values. The saver_strategy was fixed in https://github.com/ManageIQ/manageiq/pull/17168 to support String values, and the next step is to remove all Symbols from the Settings. https://github.com/ManageIQ/manageiq/issues/17201 https://bugzilla.redhat.com/show_bug.cgi?id=1558031 app/models/manageiq/providers/azure/inventory_collection_default/cloud_manager.rb | 2 +- config/settings.yml | 6 +- spec/models/manageiq/providers/azure/cloud_manager/azure_refresher_spec_common.rb | 4 +- spec/models/manageiq/providers/azure/cloud_manager/deployments_caching_spec.rb | 4 +- 4 files changed, 8 insertions(+), 8 deletions(-)
New commit detected on ManageIQ/manageiq-providers-openshift/master: https://github.com/ManageIQ/manageiq-providers-openshift/commit/e96c13f6bad7a05a493475ef0f5c2a0ffeceea87 commit e96c13f6bad7a05a493475ef0f5c2a0ffeceea87 Author: Beni Cherniavsky-Paskin <cben> AuthorDate: Wed May 2 08:05:33 2018 -0400 Commit: Beni Cherniavsky-Paskin <cben> CommitDate: Wed May 2 08:05:33 2018 -0400 Symbols in configuration values are problematic because Symbols do not roundtrip through JSON. Since the API now exposes Settings, it's not possible to set Symbols as values. The saver_strategy was fixed in ManageIQ/manageiq#17168 to support String values, and the next step is to remove all Symbols from the Settings. ManageIQ/manageiq#17201 https://bugzilla.redhat.com/show_bug.cgi?id=1558031 config/settings.yml | 2 +- spec/models/manageiq/providers/openshift/container_manager/refresher_spec.rb | 6 +- spec/models/manageiq/providers/openshift/container_manager/targeted_refresh/targeted_refresh_spec.rb | 6 +- 3 files changed, 7 insertions(+), 7 deletions(-)
FIXED. Verified on 5.10.0.23.20181106165157_92dd189.
New commit detected on ManageIQ/manageiq-providers-amazon/master: https://github.com/ManageIQ/manageiq-providers-amazon/commit/9e0fc634f42a968ecb72c9ee64227dc6d8f4a44c commit 9e0fc634f42a968ecb72c9ee64227dc6d8f4a44c Author: Beni Cherniavsky-Paskin <cben> AuthorDate: Mon Nov 12 11:08:40 2018 -0500 Commit: Beni Cherniavsky-Paskin <cben> CommitDate: Mon Nov 12 11:08:40 2018 -0500 Change saver_strategy value to String Symbols in configuration values are problematic because Symbols do not roundtrip through JSON. Since the API now exposes Settings, it's not possible to set Symbols as values. The saver_strategy was fixed in ManageIQ/manageiq#17168 to support String values, and the next step is to remove all Symbols from the Settings. ManageIQ/manageiq#17201 https://bugzilla.redhat.com/show_bug.cgi?id=1558031 config/settings.yml | 8 +- spec/models/manageiq/providers/amazon/aws_refresher_spec_common.rb | 6 +- spec/models/manageiq/providers/amazon/cloud_manager/vcr_specs/targeted_refresh_scope_spec.rb | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-)
New commit detected on ManageIQ/manageiq-providers-amazon/hammer: https://github.com/ManageIQ/manageiq-providers-amazon/commit/2c11c8ff6d2e08eec99f5ff17a47aa0617b924de commit 2c11c8ff6d2e08eec99f5ff17a47aa0617b924de Author: Ladislav Smola <lsmola> AuthorDate: Tue Nov 13 03:35:20 2018 -0500 Commit: Ladislav Smola <lsmola> CommitDate: Tue Nov 13 03:35:20 2018 -0500 Merge pull request #498 from cben/stringify_saver_strategy Change saver_strategy value to String (cherry picked from commit dd447f0114f1d076c8952ef15e7a6f587bb45ae6) https://bugzilla.redhat.com/show_bug.cgi?id=1558031 config/settings.yml | 8 +- spec/models/manageiq/providers/amazon/aws_refresher_spec_common.rb | 6 +- spec/models/manageiq/providers/amazon/cloud_manager/vcr_specs/targeted_refresh_scope_spec.rb | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-)
Some settings still don't round-trip exactly — symbols and regexps. Diff listed https://github.com/ManageIQ/manageiq/issues/17201#issuecomment-385925194 Opened PR adding regression test https://github.com/ManageIQ/manageiq/pull/18208 but need to fix all settings first... Moving back to ON_DEV, don't think it's worth verifying until fully fixed (and really the regression test will be stronger than manual verification of specific settings).
> @cben The BZ referenced in this PR isn't approved to be included in 5.9.z releases, so I'm not able to backport at this time... It has `cfme-5.9.z +` flag, no? Is something else necessary? Turns out we can fix this on 5.9 *earlier* than 5.10 (because regexps are new in 5.10). Assuming it's 5.9-destined at all, what's the process for such case? Can we get the BZ cloned for 5.9 before it's POST for 5.10?
Bug triage team decides what BZ can go to which release (e.g. 5.9.7, 5.9.8) and sets "target release" to a particular version. "cfme-5.9.z+" flag alone doesn't mean it's approved for a 5.9.z release. If there are 2 problems and one exists only in 5.10 (not fixed yet) and the other in both 5.9.z and 5.10 (already fixed in 5.10), I suggest splitting this BZ. Either way, for the fix to make a 5.9.z release, an approval from bug triage team is needed.
Forgot to mention: BZ clone happens only after it's approved for a particular z-stream release (so we don't create clones for BZs that might not make to any release).
Closing this ticket as the reported problem (advanced settings for refresh) has been fixed (merged April 2018) and will open a separate ticket to address the overall symbols / string issue.
Drew, once a new BZ is opened for symbols/string issue, please link to this BZ. Thanks!
Oopes... I meant Dennis :)
FIXED. Verified on 5.10.9.1. It also works on 5.11.0.22.