Ability to see/save Config settings for Region and Zone records.

[h-kataria]

Exposed 'Advanced' tab for Region/Zone nodes.

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

Before:

After, Note has been updated on the Server level. Exposed Advanced tab on Region and Zone level:

Dependent upon core PR: Core PR: ManageIQ/manageiq#17458

[h-kataria]

@Fryguy need help with saving of config settings for Zone/Region record, please see spec test failure

OpsController OpsController::Settings::Common #settings_update_save save config settings for selected zone
     Failure/Error: resource.set_config(config)
     
     NoMethodError:
       undefined method `set_config' for #<Zone:0x0000000034193370>
     # ./spec/manageiq/lib/vmdb/config.rb:56:in `save'
     # ./spec/manageiq/lib/vmdb/config.rb:76:in `save_file'
     # ./app/controllers/ops_controller/settings/common.rb:263:in `save_advanced_settings'
     # ./app/controllers/ops_controller/settings/common.rb:459:in `settings_update_save'
     # ./spec/controllers/ops_controller/settings/common_spec.rb:342:in `block (5 levels) in <top (required)>'

[carbonin]

To fix this spec failure, can we use resource.add_settings_for_resource(@edit[:new][:file_data]) here instead of ManageIQ/manageiq#17458 ?

[Fryguy]

I agree with @carbonin. Technically set_config is a deprecated method, and add_settings_for_resource is preferred.

Scratch that. save_file is indeed the correct method to call because it also handles validation and error reporting. save_file itself should probably be changed to not support set_config, but the way it does it now is kind of convoluted and I don't expect you to change that for this change.

So, again back to my original question...what methods are needed from MiqServer::ConfigurationManagement?

[h-kataria]

@carbonin i can't use resource.add_settings_for_resource(@edit[:new][:file_data]) directly from controller, because add_settings_for_resource does not expect settings in yaml format so i think we do need ManageIQ/manageiq#17458

[Fryguy]

I created a new backend PR ManageIQ/manageiq#17494, which handles the various nuances better.

[Fryguy]

@h-kataria With the new backend PR, you should also be able to remove the get_file and save_file references with something like the following diff:

diff --git a/app/controllers/ops_controller/settings/common.rb b/app/controllers/ops_controller/settings/common.rb
index 08bcff353..7b256ae84 100644
--- a/app/controllers/ops_controller/settings/common.rb
+++ b/app/controllers/ops_controller/settings/common.rb
@@ -253,16 +253,17 @@ module OpsController::Settings::Common
 
   def fetch_advanced_settings(resource)
     @edit = {}
-    @edit[:current] = {:file_data => VMDB::Config.get_file(resource)}
+    @edit[:current] = {:file_data => resource.settings_for_resource_yaml}
     @edit[:new] = copy_hash(@edit[:current])
     @edit[:key] = "#{@sb[:active_tab]}_edit__#{@sb[:selected_server_id]}"
     @in_a_form = true
   end
 
   def save_advanced_settings(resource)
-    result = VMDB::Config.save_file(@edit[:new][:file_data], resource) # Save the config file
-    if result != true # Result contains errors?
-      result.each do |field, msg|
+    begin
+      resource.add_settings_for_resource_yaml(@edit[:new][:file_data])
+    rescue Vmdb::Settings::ConfigurationInvalid => err
+      err.errors.each do |field, msg|
         add_flash("#{field.to_s.titleize}: #{msg}", :error)
       end
       @changed = (@edit[:new] != @edit[:current])

[h-kataria]

@Fryguy made suggested changes to use new methods from ManageIQ/manageiq#17494

[Fryguy]

core PR is merged

[Fryguy]

You still have the wrong number of indent here and on 2 lines above.

[Fryguy]

I think you need a period or exclamation point at the end of this sentence.

[Fryguy]

These 3 lines can be simplified to data = {:api => {:token_ttl => "1.day"}}

[Fryguy]

@h-kataria I made a few minor comments, and please take care of the rubocops, but otherwise this LGTM.

[Fryguy]

LGTM...Have one minor comment but I wouldn't hold up a merge for it.

[Fryguy]

Minor, but technically this logic is duplicated here and in the selected_node_parent_title. You could de-dup that logic by changing selected_node_parent_title to return nil for regions, and then there just do if selected_node_parent_title

[Fryguy]

Also minor, but this is also a duplicated pattern with the selected_node_title methods. I think this would be cleaner extracted as a selected_node method that lives with the rest in ops_helper (if possible). In this way all of the "if selected?(x_node..." logic lives in one place.

[miq-bot]

Checked commit h-kataria@c1c0bdb with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

[lgalis]

Looks good - tested saving the settings on region/server/zone.

[mzazrivec]

May I add that string interpolation will not work in gettext strings. Also, it's not usually a good idea to concatenate strings that are meant to be translated (since it complicates work for translators).

#4018