Fixes #26264, #25721 - Detect variable types on import

app/controllers/ansible_variables_controller.rb

@@ -27,11 +27,12 @@ def import
   end
 
   def confirm_import
-    results = @importer.finish_import(new_vars, old_vars)
-    info _(
+    results = @importer.finish_import(new_vars, old_vars, updated_vars)
+    success _(
       "Import of variables successfully finished.\n"\
-      "Added: #{results[:added].join(', ')} \n "\
-      "Removed: #{results[:obsolete].join(', ')}"
+      "Added: #{results[:added].count} \n "\
+      "Removed: #{results[:obsolete].count} \n"\
+      "Updated: #{results[:updated].count}"
     )
     redirect_to ansible_variables_path
   end
@@ -49,11 +50,19 @@ def resource_params
   end
 
   def new_vars
-    params[:changed] ? params[:changed][:new].as_json : {}
+    fetch_vars :new
   end
 
   def old_vars
-    params[:changed] ? params[:changed][:obsolete].as_json : {}
+    fetch_vars :obsolete
+  end
+
+  def updated_vars
+    fetch_vars :update
+  end
+
+  def fetch_vars(key)
+    params.fetch(:changed, {}).fetch(key, {}).try(:as_json) || {}
   end
 
   def import_new_roles

app/services/foreman_ansible/variables_importer.rb

@@ -6,6 +6,17 @@ module ForemanAnsible
   class VariablesImporter
     include ::ForemanAnsible::ProxyAPI
 
+    VARIABLE_TYPES = {
+      'TrueClass' => 'boolean',
+      'FalseClass' => 'boolean',
+      'Integer' => 'integer',
+      'Fixnum' => 'integer',
+      'Float' => 'real',
+      'Array' => 'array',
+      'Hash' => 'hash',
+      'String' => 'string'
+    }.freeze
+
     def initialize(proxy = nil)
       @ansible_proxy = proxy
     end
@@ -34,57 +45,63 @@ def import_new_role(role_name, new_roles)
     end
 
     def initialize_variables(variables, role)
-      variables.map do |variable|
+      variables.map do |variable_name, variable_default|
         variable = AnsibleVariable.find_or_initialize_by(
-          :key => variable
-          # :key_type, :default_value, :required
+          :key => variable_name
         )
+        if variable.new_record?
+          variable.assign_attributes(:default_value => variable_default,
+                                     :key_type => infer_key_type(variable_default))
+        end
         variable.ansible_role = role
         variable.valid? ? variable : nil
       end
     end
 
     def detect_changes(imported)
       changes = {}.with_indifferent_access
-      old, changes[:new] = imported.partition { |role| role.id.present? }
-      changes[:obsolete] = AnsibleVariable.where.not(:id => old.map(&:id))
+      persisted, changes[:new] = imported.partition { |role| role.id.present? }
+      changes[:update], _old = persisted.partition(&:changed?)
+      changes[:obsolete] = AnsibleVariable.where.not(:id => persisted.pluck(:id))
       changes
     end
 
-    def finish_import(new, obsolete)
-      results = { :added => [], :obsolete => [] }
+    def finish_import(new, obsolete, update)
+      results = { :added => [], :obsolete => [], :updated => [] }
       results[:added] = create_new_variables(new) if new.present?
       results[:obsolete] = delete_old_variables(obsolete) if obsolete.present?
+      results[:updated] = update_variables(update) if update.present?
       results
     end
 
-    def create_new_variables(new)
-      added = []
-      new.each do |role, variables|
-        variables.each_value do |variable_properties|
-          variable = AnsibleVariable.new(
-            JSON.parse(variable_properties)['ansible_variable']
-          )
-          variable.ansible_role = ::AnsibleRole.find_by(:name => role)
-          variable.save
-          added << variable.key
-        end
+    def create_new_variables(variables)
+      iterate_over_variables(variables) do |role, memo, attrs|
+        variable = AnsibleVariable.new(
+          JSON.parse(attrs)['ansible_variable']
+        )
+        variable.ansible_role = ::AnsibleRole.find_by(:name => role)
+        variable.save
+        memo << variable
       end
-      added
     end
 
-    def delete_old_variables(old)
-      removed = []
-      old.each_value do |variables|
-        variables.each_value do |variable|
-          variable = AnsibleVariable.find(
-            JSON.parse(variable)['ansible_variable']['id']
-          )
-          removed << variable.key
-          variable.destroy
-        end
+    def update_variables(variables)
+      iterate_over_variables(variables) do |_role, memo, attrs|
+        attributes = JSON.parse(attrs)['ansible_variable']
+        var = AnsibleVariable.find attributes['id']
+        var.update(attributes)
+        memo << var
+      end
+    end
+
+    def delete_old_variables(variables)
+      iterate_over_variables(variables) do |_role, memo, attrs|
+        variable = AnsibleVariable.find(
+          JSON.parse(attrs)['ansible_variable']['id']
+        )
+        memo << variable.key
+        variable.destroy
       end
-      removed
     end
 
     private
@@ -96,5 +113,18 @@ def local_variables
     def remote_variables
       proxy_api.all_variables
     end
+
+    def infer_key_type(value)
+      VARIABLE_TYPES[value.class.to_s] || 'string'
+    end
+
+    def iterate_over_variables(variables)
+      variables.reduce([]) do |memo, (role, vars)|
+        vars.map do |_key, attrs|
+          yield role, memo, attrs if block_given?
+          memo
+        end
+      end
+    end
   end
 end

app/views/ansible_variables/import.html.erb

@@ -9,6 +9,9 @@
   <%= link_to_function(icon_text("check", _("Obsolete")),
                        "toggleCheckboxesBySelector('.variable_select_boxes_obsolete')",
                        :title => _("Check/Uncheck obsolete")) %>
+  <%= link_to_function(icon_text("check", _("Update")),
+                       "toggleCheckboxesBySelector('.variable_select_boxes_update')",
+                       :title => _("Check/Uncheck update")) %>
   </h6>
   <table class="<%= table_css_classes %>">
     <thead>
@@ -39,7 +42,7 @@
             <td><%= variable.ansible_role.hosts.count %></td>
             <td><%= variable.ansible_role.hostgroups.count %></td>
             <td>
-              <%= { "new" => _("Add"), "obsolete" => _("Remove") }[kind] %>
+              <%= { "new" => _("Add"), "obsolete" => _("Remove"), "update" => _("Update") }[kind] %>
             </td>
           </tr>
         <% end %>

test/unit/services/ansible_variables_importer_test.rb

@@ -12,8 +12,8 @@ class AnsibleVariablesImporterTest < ActiveSupport::TestCase
     already_existing = FactoryBot.create(:ansible_variable)
     new_role = FactoryBot.create(:ansible_role)
     api_response = {
-      new_role.name => ['new_var'],
-      already_existing.ansible_role.name => [already_existing.key]
+      new_role.name => { 'new_var' => 'new value' },
+      already_existing.ansible_role.name => { already_existing.key => already_existing.default_value }
     }
     changes = @importer.import_variables(api_response, [new_role.name])
     assert_not_empty changes['new']
@@ -34,6 +34,17 @@ class AnsibleVariablesImporterTest < ActiveSupport::TestCase
 
   test 'does not do anything if response is empty' do
     changes = @importer.import_variables({}, [])
-    assert_equal({ 'new' => [], 'obsolete' => [] }, changes)
+    assert_equal({ 'new' => [], 'obsolete' => [], 'update' => [] }, changes)
+  end
+
+  test 'do not update changed defaults' do
+    role = FactoryBot.create(:ansible_role)
+    variable = FactoryBot.create(:ansible_variable, :default_value => 'default value', :ansible_role_id => role.id)
+    api_response = {
+      role.name => { variable.key => 'changed value', 'new_variable' => 'new value' }
+    }
+    changes = @importer.import_variables(api_response, [])
+    assert_empty changes['update']
+    assert_equal 'new_variable', changes['new'].first.key
   end
 end