diff --git a/.rubocop.yml b/.rubocop.yml index cad87717..628dc0db 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -16,7 +16,7 @@ Metrics/MethodLength: Max: 20 Metrics/LineLength: - Enabled: false + Enabled: false Style/Next: Enabled: false @@ -98,6 +98,9 @@ Metrics/BlockLength: Metrics/PerceivedComplexity: Max: 20 +Metrics/ParameterLists: + Max: 6 + Lint/UnusedMethodArgument: Enabled: false diff --git a/app/controllers/api/v2/template_controller.rb b/app/controllers/api/v2/template_controller.rb index f033ec1f..3fe5e6aa 100644 --- a/app/controllers/api/v2/template_controller.rb +++ b/app/controllers/api/v2/template_controller.rb @@ -9,6 +9,7 @@ class TemplateController < ::Api::V2::BaseController param :filter, String, :required => false, :desc => N_("Import names matching this regex (case-insensitive; snippets are not filtered).") param :negate, :bool, :required => false, :desc => N_("Negate the prefix (for purging).") param :associate, String, :required => false, :desc => N_("Associate to OS's, Locations & Organizations. Options are: always, new or never.") + param :force, :bool, :required => false, :desc => N_("Update templates that are locked") def import results = ForemanTemplates::TemplateImporter.new({ verbose: params['verbose'], @@ -19,6 +20,7 @@ def import filter: params['filter'], associate: params['associate'], negate: params['negate'], + force: params['force'] }).import! render :json => { :message => results } end diff --git a/app/models/concerns/foreman_templates/provisioning_template_import.rb b/app/models/concerns/foreman_templates/provisioning_template_import.rb index 27fc09a6..ad227b54 100644 --- a/app/models/concerns/foreman_templates/provisioning_template_import.rb +++ b/app/models/concerns/foreman_templates/provisioning_template_import.rb @@ -3,9 +3,9 @@ module ProvisioningTemplateImport extend ActiveSupport::Concern module ClassMethods - def import!(name, text, metadata) + def import!(name, text, metadata, force = false) # Check for snippet type - return import_snippet!(name, text) if metadata['snippet'] || metadata['kind'] == 'snippet' + return import_snippet!(name, text, force) if metadata['snippet'] || metadata['kind'] == 'snippet' # Get template type kind = TemplateKind.find_by(name: metadata['kind']) @@ -23,39 +23,71 @@ def import!(name, text, metadata) organizations = map_metadata(metadata, 'organizations') # Printout helpers - c_or_u = template.new_record? ? 'Created' : 'Updated' - id_string = ('id' + template.id) rescue '' + c_or_u = template.new_record? ? 'Creating' : 'Updating' + id_string = template.new_record? ? '' : "id #{template.id}" + if template.locked? && !template.new_record? && !force + return { :diff => nil, + :status => false, + :result => "Skipping Template #{id_string}:#{name} - template is locked" } + end + + associate_metadata data, template, metadata, oses, organizations, locations + + diff = nil + status = nil + if template_changed?(data, template) + diff = create_diff(data, template) + template.ignore_locking do + status = template.update_attributes(data) + end + result = build_associations_result c_or_u, id_string, name, oses, organizations, locations + else + status = true + result = " No change to Template #{id_string}:#{name}" + end + { :diff => diff, :status => status, :result => result, :errors => template.errors } + end + + def associate_metadata(data, template, metadata, oses, organizations, locations) if (metadata['associate'] == 'new' && template.new_record?) || (metadata['associate'] == 'always') data[:operatingsystem_ids] = oses.map(&:id) data[:location_ids] = locations.map(&:id) data[:organization_ids] = organizations.map(&:id) end + data + end - if data[:template] != template.template - diff = Diffy::Diff.new( + def build_associations_result(c_or_u, id_string, name, oses, organizations, locations) + res = " #{c_or_u} Template #{id_string}:#{name}" + res += "\n Operatingsystem Associations:\n - #{oses.map(&:fullname).join("\n - ")}" unless oses.empty? + res += "\n Organizations Associations:\n - #{organizations.map(&:name).join("\n - ")}" unless organizations.empty? + res += "\n Location Associations:\n - #{locations.map(&:name).join("\n - ")}" unless locations.empty? + res + end + + def create_diff(data, template) + if template_content_changed?(template.template, data[:template]) + Diffy::Diff.new( template.template, data[:template], :include_diff_info => true ).to_s(:color) - status = template.update_attributes(data) - result = " #{c_or_u} Template #{id_string}:#{name}" - result += "\n Operatingsystem Associations:\n - #{oses.map(&:fullname).join("\n - ")}" unless oses.empty? - result += "\n Organizations Associations:\n - #{organizations.map(&:name).join("\n - ")}" unless organizations.empty? - result += "\n Location Associations:\n - #{locations.map(&:name).join("\n - ")}" unless locations.empty? - elsif data[:operatingsystem_ids] || data[:location_ids] || data[:organization_ids] - diff = nil - status = template.update_attributes(data) - result = " #{c_or_u} Template Associations #{id_string}:#{name}" - result += "\n Operatingsystem Associations:\n - #{oses.map(&:fullname).join("\n - ")}" unless oses.empty? - result += "\n Organizations Associations:\n - #{organizations.map(&:name).join("\n - ")}" unless organizations.empty? - result += "\n Location Associations:\n - #{locations.map(&:name).join("\n - ")}" unless locations.empty? else - diff = nil - status = true - result = " No change to Template #{id_string}:#{name}" + nil end - { :diff => diff, :status => status, :result => result } + end + + def template_content_changed?(template_template, data_template) + template_template != data_template + end + + def associations_changed?(data) + !(data[:operatingsystem_ids] || data[:location_ids] || data[:organization_ids]).nil? + end + + def template_changed?(data, template) + template_content_changed?(template.template, data[:template]) || associations_changed?(data) end end end diff --git a/app/models/concerns/foreman_templates/ptable_import.rb b/app/models/concerns/foreman_templates/ptable_import.rb index bcc1fa4a..9731791a 100644 --- a/app/models/concerns/foreman_templates/ptable_import.rb +++ b/app/models/concerns/foreman_templates/ptable_import.rb @@ -3,9 +3,9 @@ module PtableImport extend ActiveSupport::Concern module ClassMethods - def import!(name, text, metadata) + def import!(name, text, metadata, force = false) # Check for snippet type - return import_snippet!(name, text) if metadata['snippet'] + return import_snippet!(name, text, force) if metadata['snippet'] # Data ptable = Ptable.where(:name => name).first_or_initialize @@ -17,42 +17,72 @@ def import!(name, text, metadata) organizations = map_metadata(metadata, 'organizations') # Printout helpers - c_or_u = ptable.new_record? ? 'Created' : 'Updated' - id_string = ('id' + ptable.id) rescue '' + c_or_u = ptable.new_record? ? 'Creating' : 'Updating' + id_string = ptable.new_record? ? '' : "id #{ptable.id}" + if ptable.locked? && !ptable.new_record? && !force + return { :diff => nil, + :status => false, + :result => "Skipping Partition Table #{id_string}:#{name} - partition table is locked" } + end + + associate_metadata data, ptable, metadata, oses, organizations, locations + + diff = nil + status = nil + if ptable_changed?(data, ptable) + diff = create_diff(data, ptable) + ptable.ignore_locking do + status = ptable.update_attributes(data) + end + result = build_associations_result c_or_u, id_string, name, oses, organizations, locations + else + status = true + result = " No change to Ptable #{id_string}:#{name}" + end + { :diff => diff, :status => status, :result => result, :errors => ptable.errors } + end + def associate_metadata(data, ptable, metadata, oses, organizations, locations) if (metadata['associate'] == 'new' && ptable.new_record?) || (metadata['associate'] == 'always') data[:operatingsystem_ids] = oses.map(&:id) data[:os_family] = oses.map(&:family).uniq.first data[:location_ids] = locations.map(&:id) data[:organization_ids] = organizations.map(&:id) end + data + end - if data[:layout] != ptable.layout - diff = Diffy::Diff.new( + def ptable_content_changed?(data_layout, ptable_layout) + data_layout != ptable_layout + end + + def associations_changed?(data) + !(data[:os_family] || data[:location_ids] || data[:organization_ids]).nil? + end + + def create_diff(data, ptable) + if ptable_content_changed?(data[:layout], ptable.layout) + Diffy::Diff.new( ptable.layout, data[:layout], :include_diff_info => true ).to_s(:color) - status = ptable.update_attributes(data) - result = " #{c_or_u} Ptable #{id_string}:#{name}" - result += "\n Operatingsystem Family:\n - #{oses.map(&:family).uniq.first}" unless oses.empty? - result += "\n Operatingsystem Associations:\n - #{oses.map(&:fullname).join("\n - ")}" unless oses.empty? - result += "\n Organizations Associations:\n - #{organizations.map(&:name).join("\n - ")}" unless organizations.empty? - result += "\n Location Associations:\n - #{locations.map(&:name).join("\n - ")}" unless locations.empty? - elsif data[:os_family] || data[:location_ids] || data[:organization_ids] - diff = nil - status = ptable.update_attributes(data) - result = " #{c_or_u} Ptable Associations #{id_string}:#{name}" - result += "\n Operatingsystem Family:\n - #{oses.map(&:family).uniq.first}" unless oses.empty? - result += "\n Operatingsystem Associations:\n - #{oses.map(&:fullname).join("\n - ")}" unless oses.empty? - result += "\n Organizations Associations:\n - #{organizations.map(&:name).join("\n - ")}" unless organizations.empty? - result += "\n Location Associations:\n - #{locations.map(&:name).join("\n - ")}" unless locations.empty? else - diff = nil - status = true - result = " No change to Ptable #{id_string}:#{name}" + nil end - { :diff => diff, :status => status, :result => result } + end + + def build_associations_result(c_or_u, id_string, name, oses, organizations, locations) + res = " #{c_or_u} Ptable #{id_string}:#{name}" + res += "\n Operatingsystem Family:\n - #{oses.map(&:family).uniq.first}" unless oses.empty? + res += "\n Operatingsystem Associations:\n - #{oses.map(&:fullname).join("\n - ")}" unless oses.empty? + res += "\n Organizations Associations:\n - #{organizations.map(&:name).join("\n - ")}" unless organizations.empty? + res += "\n Location Associations:\n - #{locations.map(&:name).join("\n - ")}" unless locations.empty? + res + end + + def ptable_changed?(data, ptable) + ptable_content_changed?(data[:layout], ptable.layout) || associations_changed?(data) end end end diff --git a/app/models/concerns/foreman_templates/template_import.rb b/app/models/concerns/foreman_templates/template_import.rb index ffce5d30..b2050218 100644 --- a/app/models/concerns/foreman_templates/template_import.rb +++ b/app/models/concerns/foreman_templates/template_import.rb @@ -3,7 +3,7 @@ module TemplateImport extend ActiveSupport::Concern module ClassMethods - def import_snippet!(name, text) + def import_snippet!(name, text, force = false) # Data snippet = self.where(:name => name).first_or_initialize data = { @@ -12,23 +12,32 @@ def import_snippet!(name, text) } # Printout helpers - c_or_u = snippet.new_record? ? 'Created' : 'Updated' - id_string = ('id' + snippet.id) rescue '' + c_or_u = snippet.new_record? ? 'Creating' : 'Updating' + id_string = snippet.new_record? ? '' : "id #{snippet.id}" + if snippet.locked? && !snippet.new_record? && !force + return { :diff => nil, + :status => false, + :result => "Skipping snippet #{id_string}:#{name} - template is locked" } + end + + status = nil if data[:template] != snippet.template diff = Diffy::Diff.new( snippet.template, data[:template], :include_diff_info => true ).to_s(:color) - status = snippet.update_attributes(data) + snippet.ignore_locking do + status = snippet.update_attributes(data) + end result = " #{c_or_u} Snippet #{id_string}:#{name}" else diff = nil status = true result = " No change to Snippet #{id_string}:#{name}" end - { :diff => diff, :status => status, :result => result } + { :diff => diff, :status => status, :result => result, :errors => snippet.errors } end def map_metadata(metadata, param) @@ -40,11 +49,11 @@ def map_metadata(metadata, param) end.flatten.compact when 'locations' metadata[param].map do |loc| - Location.all.map { |db| db.name =~ /^#{loc}/ ? db : nil } + User.current.my_locations.map { |db| db.name =~ /^#{loc}/ ? db : nil } end.flatten.compact when 'organizations' metadata[param].map do |org| - Organization.all.map { |db| db.name =~ /^#{org}/ ? db : nil } + User.current.my_organizations.map { |db| db.name =~ /^#{org}/ ? db : nil } end.flatten.compact end else diff --git a/app/models/setting/template_sync.rb b/app/models/setting/template_sync.rb index 1baa8402..45c069dc 100644 --- a/app/models/setting/template_sync.rb +++ b/app/models/setting/template_sync.rb @@ -31,7 +31,8 @@ def self.load_defaults self.set('template_sync_repo', N_('Default Git repo to sync from'), 'https://github.com/theforeman/community-templates.git', N_('Repo')), self.set('template_sync_negate', N_('Negate the prefix (for purging) / filter (for importing/exporting)'), false, N_('Negate')), self.set('template_sync_branch', N_('Default branch in Git repo'), nil, N_('Branch')), - self.set('template_sync_metadata_export_mode', N_('Default metadata export mode, refresh re-renders metadata, keep will keep existing metadata, remove exports template withou metadata'), 'refresh', N_('Metadata export mode'), nil, { :collection => Proc.new { self.metadata_export_mode_types } }) + self.set('template_sync_metadata_export_mode', N_('Default metadata export mode, refresh re-renders metadata, keep will keep existing metadata, remove exports template withou metadata'), 'refresh', N_('Metadata export mode'), nil, { :collection => Proc.new { self.metadata_export_mode_types } }), + self.set('template_sync_force', N_('Should importing overwrite locked templates?'), false, N_('Force import')), ].compact.each { |s| self.create! s.update(:category => "Setting::TemplateSync") } end diff --git a/app/services/foreman_templates/template_importer.rb b/app/services/foreman_templates/template_importer.rb index ea750eaf..3c97d712 100644 --- a/app/services/foreman_templates/template_importer.rb +++ b/app/services/foreman_templates/template_importer.rb @@ -6,15 +6,13 @@ class TemplateImporter < Action attr_accessor :metadata, :name, :text def self.setting_overrides - super + %i(associate) + super + %i(associate force) end def initialize(args = {}) super - # Rake hands off strings, not booleans, and "false" is true... - if @verbose.is_a?(String) - @verbose = @verbose != 'false' - end + @verbose = parse_bool(@verbose) + @force = parse_bool(@force) end def import! @@ -69,19 +67,19 @@ def parse_files! end begin - # Expects a return of { :diff, :status, :result } + # Expects a return of { :diff, :status, :result, :errors } data = if metadata['model'].present? - metadata['model'].constantize.import!(name, text, metadata) + metadata['model'].constantize.import!(name, text, metadata, @force) else # For backwards-compat before "model" metadata was added case metadata['kind'] when 'ptable' - Ptable.import!(name, text, metadata) + Ptable.import!(name, text, metadata, @force) when 'job_template' # TODO: update REX templates to have `model` and delete this update_job_template(name, text) else - ProvisioningTemplate.import!(name, text, metadata) + ProvisioningTemplate.import!(name, text, metadata, @force) end end @@ -89,12 +87,12 @@ def parse_files! data[:diff] = calculate_diff(data[:old], data[:new]) end - if data[:status] == true && @verbose + if @verbose result_lines << data[:result] result_lines << data[:diff] unless data[:diff].nil? - elsif data[:status] == false - result_lines << "Template \"#{name}\": #{data[:result]}" end + result_lines << status_to_text(data[:status], name) + result_lines << data[:errors] unless data[:errors].empty? rescue MissingKindError result_lines << " Skipping: '#{name}' - No template kind or model detected" next @@ -165,5 +163,21 @@ def purge! template.destroy end end # :purge + + private + + def parse_bool(bool_name) + bool_name.is_a?(String) ? bool_name != 'false' : bool_name + end + + def status_to_text(status, name) + msg = "#{name} - import " + msg << if status + "success" + else + 'failure' + end + msg + end end end diff --git a/test/templates/locking/core_initial/metadata1.erb b/test/templates/locking/core_initial/metadata1.erb new file mode 100644 index 00000000..62bced97 --- /dev/null +++ b/test/templates/locking/core_initial/metadata1.erb @@ -0,0 +1,10 @@ +something +<%# +kind: provision +name: Test Data +%> +foo + +bar +<%# another comment %> +quux diff --git a/test/templates/locking/core_initial/ptable1.erb b/test/templates/locking/core_initial/ptable1.erb new file mode 100644 index 00000000..c430a487 --- /dev/null +++ b/test/templates/locking/core_initial/ptable1.erb @@ -0,0 +1,5 @@ +<%# +kind: ptable +name: Test Ptable +%> +test ptable layout diff --git a/test/templates/locking/core_initial/snippet1.erb b/test/templates/locking/core_initial/snippet1.erb new file mode 100644 index 00000000..56a36903 --- /dev/null +++ b/test/templates/locking/core_initial/snippet1.erb @@ -0,0 +1,5 @@ +<%# +kind: snippet +name: Test Snippet +%> +some snippet text diff --git a/test/templates/locking/core_updated/metadata1.erb b/test/templates/locking/core_updated/metadata1.erb new file mode 100644 index 00000000..a2a63786 --- /dev/null +++ b/test/templates/locking/core_updated/metadata1.erb @@ -0,0 +1,11 @@ +something +<%# +kind: provision +name: Test Data +%> +foo +baz +bar +<%# another comment %> +quux +small squid diff --git a/test/templates/locking/core_updated/ptable1.erb b/test/templates/locking/core_updated/ptable1.erb new file mode 100644 index 00000000..af4c31b7 --- /dev/null +++ b/test/templates/locking/core_updated/ptable1.erb @@ -0,0 +1,5 @@ +<%# +kind: ptable +name: Test Ptable +%> +test ptable layout updated diff --git a/test/templates/locking/core_updated/snippet1.erb b/test/templates/locking/core_updated/snippet1.erb new file mode 100644 index 00000000..01ff9a1e --- /dev/null +++ b/test/templates/locking/core_updated/snippet1.erb @@ -0,0 +1,5 @@ +<%# +kind: snippet +name: Test Snippet +%> +some snippet text with crazy modifications diff --git a/test/unit/template_importer_test.rb b/test/unit/template_importer_test.rb index e7aeef39..c8f9ead7 100644 --- a/test/unit/template_importer_test.rb +++ b/test/unit/template_importer_test.rb @@ -64,13 +64,13 @@ def importer(opts = {}) # Check plugin template was not imported ct = Template.find_by(name: 'FooBar PluginTest') refute ct.present? - assert_equal results, [" Skipping: 'FooBar PluginTest' - Unknown template model 'ForemanTemplates::TemplateImporterTest::TestTemplate'"] + assert_includes results, " Skipping: 'FooBar PluginTest' - Unknown template model 'ForemanTemplates::TemplateImporterTest::TestTemplate'" end test 'can extend without changes' do # Test template class class TestTemplate < ::Template - def self.import!(name, text, _metadata) + def self.import!(name, text, _metadata, force = false) audited # core tries to call :audit_comment, breaks without this template = TestTemplate.new(:name => name, :template => text) template.save! @@ -245,6 +245,71 @@ def self.import!(name, text, _metadata) assert_equal 'FooBar template FooBar something', @importer.auto_prefix('template FooBar something') end + test "should not update locked templates" do + initial_path = "#{@engine_root}/test/templates/locking/core_initial" + template_template = File.read("#{initial_path}/metadata1.erb") + ptable_layout = File.read("#{initial_path}/ptable1.erb") + snippet_template = File.read("#{initial_path}/snippet1.erb") + + provision = TemplateKind.find_by :name => 'provision' + template = FactoryGirl.create(:provisioning_template, + :name => "Test Data", + :template => template_template, + :locked => true, + :template_kind => provision) + ptable = FactoryGirl.create(:ptable, :name => "Test Ptable", :locked => true, :layout => ptable_layout) + snippet = FactoryGirl.create(:provisioning_template, :snippet, :name => "Test Snippet", :locked => true, :template => snippet_template) + + imp = importer(:dirname => '/test/templates/locking/core_updated', :verbose => true, :prefix => '') + res = imp.import! + + assert res.include?("Skipping Template id #{template.id}:#{template.name} - template is locked") + assert res.include?("Skipping Partition Table id #{ptable.id}:#{ptable.name} - partition table is locked") + assert res.include?("Skipping snippet id #{snippet.id}:#{snippet.name} - template is locked") + assert_equal template_template, template.template + assert_equal ptable_layout, ptable.layout + assert_equal snippet_template, snippet.template + end + + test "should update locked template when forced" do + locking_path = "#{@engine_root}/test/templates/locking" + initial_path = "#{locking_path}/core_initial" + template_template = File.read("#{initial_path}/metadata1.erb") + ptable_layout = File.read("#{initial_path}/ptable1.erb") + snippet_template = File.read("#{initial_path}/snippet1.erb") + + provision = TemplateKind.find_by :name => 'provision' + template = FactoryGirl.create(:provisioning_template, + :name => "Test Data", + :template => template_template, + :locked => true, + :template_kind => provision) + ptable = FactoryGirl.create(:ptable, :name => "Test Ptable", :locked => true, :layout => ptable_layout) + snippet = FactoryGirl.create(:provisioning_template, :snippet, :name => "Test Snippet", :locked => true, :template => snippet_template) + + imp = importer(:dirname => '/test/templates/locking/core_updated', :verbose => true, :prefix => '', :force => true) + res = imp.import! + + updated_path = "#{locking_path}/core_updated" + new_template_template = File.read("#{updated_path}/metadata1.erb") + new_ptable_layout = File.read("#{updated_path}/ptable1.erb") + new_snippet_template = File.read("#{updated_path}/snippet1.erb") + + [template, snippet, ptable].map(&:reload) + + refute_equal template_template, template.template + assert_equal new_template_template, template.template + assert res.include? " Updating Template id #{template.id}:#{template.name}" + + refute_equal ptable_layout, ptable.layout + assert_equal new_ptable_layout, ptable.layout + assert res.include? " Updating Ptable id #{ptable.id}:#{ptable.name}" + + refute_equal snippet_template, snippet.template + assert_equal new_snippet_template, snippet.template + assert res.include? " Updating Snippet id #{snippet.id}:#{snippet.name}" + end + private def setup_settings(opts = {})