Ability to reset settings to default value and delete newly added keys #17482

yrudman · 2018-05-25T21:43:24Z

Current implementation of Settings does not allow to reset value to default or delete new entries.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1576984

This PR introduced "magic" words:
<<reset>> - if this word set as value than key/value deleted from settings_changes table, which effectively set that value to default (default value loaded from settings.yaml.
To reset all subtrees down from some node: delete all subtree in UI editor and make that node a leaf with value <<reset>>>
<<reset_all>>> - works the same as <<reset>> but on ALL resources

@miq-bot add-label bug

If <<reset>> word passed as value for node or leaf than that node or leaf deleted for selected resource. If <<reset_all> passed that leaf/node deleted for all resorces. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1576984

carbonin · 2018-06-04T18:03:22Z

lib/vmdb/settings.rb

+      hash.each do |key, value|
+        key_path = path.dup << key
+        yield key, value, key_path, hash
+        walk_hash(value, key_path, &block) if value&.class == hash.class


Won't hash.class just always be Hash? Also do we want to recurse into something like an array of hashes? I think this would fail in that case.

@carbonin it is always Hash, will change.
Array of Hashes would be OK, I am adding another rspec test for it ...

carbonin · 2018-06-04T18:04:14Z

lib/vmdb/settings.rb

+        _log.info("removing custom settings #{delta[:key]} on #{resource.class} id:#{resource.id}")
+        resource.settings_changes.where("key LIKE ?", "#{delta[:key]}%").destroy_all
+      when DELETE_ALL_COMMAND
+        _log.info("resetting #{delta[:key]} to defaul for all resorces")


Typo s/defaul/default/

carbonin · 2018-06-04T18:08:55Z

lib/vmdb/settings.rb

+    end
+    private_class_method :process_magic_value
+
+    def self.walk_hash(hash, path = [], &block)


I'm not sure I understand why we are keeping track of this path parameter here.
We are yielding it and passing it along into the next level of recursion, but don't use it in the block in .remove_magic_values

you are right, we do not need it here if we are ignoring Arrays
I was thinking to process Arrays too (as in SettingsWalker from were this method was copied), but could not find example in our settings.yml and decided not top do so.
Removing it ...

@Fryguy should we accommodate resetting nested Hash as an element of Array in Settings ?

carbonin · 2018-06-04T19:25:06Z

lib/vmdb/settings.rb

@@ -21,6 +22,11 @@ def initialize(errors)
    PASSWORD_FIELDS = Vmdb::SettingsWalker::PASSWORD_FIELDS
    DUMP_LOG_FILE   = Rails.root.join("log/last_settings.txt").freeze

+    # RESET_COMMAND remove record for selected key and specific resource
+    # RESET_ALL_COMMAND remove all records for selected key and all resources


I think these should match the name of the constant, right? DELETE_COMMAND and DELETE_ALL_COMMAND

gtanzillo · 2018-06-04T19:42:41Z

lib/vmdb/settings.rb

+      return false unless MAGIC_VALUES.include?(delta[:value])
+      case delta[:value]
+      when DELETE_COMMAND
+        _log.info("removing custom settings #{delta[:key]} on #{resource.class} id:#{resource.id}")


I think this should read resetting ... to be consistent with <reset_all>

…presentation

miq-bot · 2018-06-04T20:02:42Z

Checked commits yrudman/manageiq@9888390~...c68a4fe with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

gtanzillo

LGTM 👍

Fryguy · 2018-06-08T16:22:27Z

lib/vmdb/settings.rb

+    # RESET_COMMAND remove record for selected key and specific resource
+    # RESET_ALL_COMMAND remove all records for selected key and all resources
+    MAGIC_VALUES = [RESET_COMMAND      = "<<reset>>".freeze,
+                    RESET_ALL_COMMAND  = "<<reset_all>>".freeze].freeze


The definitions inside other definitions is weird, but...

Fryguy · 2018-06-08T19:49:17Z

lib/vmdb/settings.rb

+    private_class_method :process_magic_value
+
+    def self.walk_hash(hash, &block)
+      hash.each do |key, value|


Prefer using the existing walk method, which already handles this. Even so, I don't think Iike this manual thing at all, A reset is really just taking the parent's value at that level and replacing it. Then, let the differ handle the rest.

Fryguy · 2018-06-08T19:51:18Z

@yrudman Sorry it took me a while to review this, but the general approach is wrong, IMO. Resetting to default is equivalent to settings the values specifically to the parent value. We already have logic for walking a hash and we have logic for dealing with applying changes when the user re-specifies the default. So, I think that it should be implement as a walk to find magic values, and then just replacing that section with the parent's corresponding values.

Fryguy · 2018-06-08T19:54:51Z

I am going to merge this for now, to get the feature moving, but I'll make a follow up PR to clean up the approach

Fryguy · 2018-06-08T23:48:06Z

@yrudman I started writing the fixup PR for this, and it's pretty straight forward, but the reset_all doesn't work as I expected. What are you expecting it to do when someone sends <<reset_all>> at each of the MiqServer, Zone, and MiqRegion levels? From the way you wrote the code, it literally removes everything from every region, which is just wrong. Were you trying to implement that at the zone and region it would delete all of the overrides from the children?

yrudman · 2018-06-09T14:31:18Z

@Fryguy <<reset_all>>```` supposed to reset selected settings to values we supply (values from yml``` file).

yrudman force-pushed the allow-delete-settings branch from f24c42b to 8a3fc77 Compare May 25, 2018 21:45

miq-bot added the wip label May 25, 2018

yrudman force-pushed the allow-delete-settings branch 9 times, most recently from 4a1c87a to 108c99c Compare June 1, 2018 13:23

yrudman added 3 commits June 1, 2018 10:12

rspec coverage for <<reset>> and <<reset_all>> magic words

9888390

added rspec to check if magic words passing validations

45beae8

add magic words <<reset>>, <<rest_all>>.

5d1861e

If <<reset>> word passed as value for node or leaf than that node or leaf deleted for selected resource. If <<reset_all> passed that leaf/node deleted for all resorces. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1576984

yrudman force-pushed the allow-delete-settings branch from 108c99c to 5d1861e Compare June 1, 2018 14:19

yrudman changed the title ~~[WIP] Ability to reset settings to default value, delete newly added keys and set a value to nil~~ Ability to reset settings to default value and delete newly added keys Jun 1, 2018

miq-bot removed the wip label Jun 1, 2018

miq-bot assigned Fryguy Jun 1, 2018

carbonin reviewed Jun 4, 2018

View reviewed changes

yrudman added 2 commits June 4, 2018 15:18

added rspec to test deleting array

b960144

remove not used parameter

76b928f

carbonin reviewed Jun 4, 2018

View reviewed changes

gtanzillo reviewed Jun 4, 2018

View reviewed changes

renaming constant from DELETE_... to RESET_... to match log and ui re…

c68a4fe

…presentation

gtanzillo approved these changes Jun 4, 2018

View reviewed changes

Fryguy reviewed Jun 8, 2018

View reviewed changes

Fryguy merged commit 1bc038a into ManageIQ:master Jun 8, 2018

Fryguy added the enhancement label Jun 8, 2018

Fryguy added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 8, 2018

Fryguy added the core label Jun 8, 2018

Fryguy mentioned this pull request Jun 9, 2018

Remove <<reset_all>> magic value from Settings #17563

Merged

yrudman deleted the allow-delete-settings branch June 11, 2018 14:31

Fryguy mentioned this pull request Aug 31, 2021

Removing a value in Advanced Server Configuration is not storing ManageIQ/manageiq-ui-classic#835

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to reset settings to default value and delete newly added keys #17482

Ability to reset settings to default value and delete newly added keys #17482

yrudman commented May 25, 2018 •

edited

Loading

carbonin Jun 4, 2018

yrudman Jun 4, 2018

carbonin Jun 4, 2018

carbonin Jun 4, 2018

yrudman Jun 4, 2018

carbonin Jun 4, 2018

yrudman Jun 4, 2018

gtanzillo Jun 4, 2018

yrudman Jun 4, 2018

miq-bot commented Jun 4, 2018

gtanzillo left a comment

Fryguy Jun 8, 2018

Fryguy Jun 8, 2018

Fryguy commented Jun 8, 2018

Fryguy commented Jun 8, 2018

Fryguy commented Jun 8, 2018 •

edited

Loading

yrudman commented Jun 9, 2018

Ability to reset settings to default value and delete newly added keys #17482

Ability to reset settings to default value and delete newly added keys #17482

Conversation

yrudman commented May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jun 4, 2018

gtanzillo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Jun 8, 2018

Fryguy commented Jun 8, 2018

Fryguy commented Jun 8, 2018 • edited Loading

yrudman commented Jun 9, 2018

yrudman commented May 25, 2018 •

edited

Loading

Fryguy commented Jun 8, 2018 •

edited

Loading