Override available_template_kinds to return only templates that are d…

…efined in the repository

.rubocop.yml

@@ -1,5 +1,5 @@
 AllCops:
-  TargetRubyVersion: 2.3
+  TargetRubyVersion: 2.5
   TargetRailsVersion: 5.1
   Exclude:
     - 'Rakefile'

.travis.yml

@@ -1,6 +1,6 @@
 language: ruby
 rvm:
-  - 2.3
+  - 2.5
 install:
   - unset BUNDLE_GEMFILE
   - cd ..

app/models/concerns/foreman_git_templates/hostext/operating_system.rb

@@ -11,6 +11,18 @@ def provisioning_template(opts = {})
           kind = opts[:kind] || 'provision'
           Template.new(name: kind)
         end
+
+        def available_template_kinds(provisioning = nil)
+          return super unless host_params['template_url']
+
+          repository_path = RepositoryFetcher.call(host_params['template_url'])
+          template_kinds(provisioning).map do |kind|
+            RepositoryReader.call(repository_path, kind.name)
+            Template.new(name: kind.name)
+          rescue RepositoryReader::FileUnreadableError # file is missing or empty
+            next
+          end.compact
+        end
       end
 
       included do

app/services/foreman_git_templates/repository_reader.rb

@@ -8,7 +8,7 @@ def initialize(repository_path, file)
     end
 
     def call
-      raise FileUnreadableError, "Cannot read #{file} from repository" if content.nil?
+      raise MissingFileError, "The #{file} file is missing" if content.nil?
       content
     end
 
@@ -21,17 +21,22 @@ def self.call(repository_path, file)
     class RepositoryReaderError < StandardError; end
     class RepositoryUnreadableError < RepositoryReaderError; end
     class FileUnreadableError < RepositoryReaderError; end
+    class MissingFileError < FileUnreadableError; end
+    class EmptyFileError < FileUnreadableError; end
 
     attr_reader :repository_path, :file
 
     def content
-      @content ||= begin
-        Tar.untar(repository_path) do |tar|
-          return tar.each { |entry| break entry.read if entry.file? && matched_with_file_or_directory?(entry.full_name) }
+      @content ||= Tar.untar(repository_path) do |tar|
+        return tar.each do |entry|
+          next if !entry.file? || !matched_with_file_or_directory?(entry.full_name)
+          content = entry.read
+          raise EmptyFileError, "The #{file} file is empty" if content.nil?
+          break content
         end
-      rescue Errno::ENOENT
-        raise RepositoryUnreadableError, "Cannot read repository from #{repository_path}"
       end
+    rescue Errno::ENOENT
+      raise RepositoryUnreadableError, "Cannot read repository from #{repository_path}"
     end
 
     def matched_with_file_or_directory?(path)

lib/foreman_git_templates/engine.rb

@@ -15,12 +15,10 @@ class Engine < ::Rails::Engine
 
     # Include concerns in this config.to_prepare block
     config.to_prepare do
-      begin
-        Foreman::Renderer.singleton_class.prepend(ForemanGitTemplates::Renderer)
-        Host::Managed.include(ForemanGitTemplates::Hostext::OperatingSystem)
-      rescue StandardError => e
-        Rails.logger.warn "ForemanGitTemplates: skipping engine hook (#{e})"
-      end
+      Foreman::Renderer.singleton_class.prepend(ForemanGitTemplates::Renderer)
+      Host::Managed.include(ForemanGitTemplates::Hostext::OperatingSystem)
+    rescue StandardError => e
+      Rails.logger.warn "ForemanGitTemplates: skipping engine hook (#{e})"
     end
 
     initializer 'foreman_git_templates.register_gettext', after: :load_config_initializers do |_app|

test/controller/unattended_controller_test.rb

@@ -1,65 +1,51 @@
 # frozen_string_literal: true
 
-require 'test_helper'
+require 'test_plugin_helper'
 
 class UnattendedControllerTest < ActionController::TestCase
-  setup do
-    org = FactoryBot.create(:organization, ignore_types: ['ProvisioningTemplate'])
-    loc = FactoryBot.create(:location, ignore_types: ['ProvisioningTemplate'])
-    ptable = FactoryBot.create(:ptable, name: 'default', operatingsystem_ids: [operatingsystems(:redhat).id])
-    @host = FactoryBot.create(:host, :managed, :with_template_url,
-                              operatingsystem: operatingsystems(:redhat),
-                              ptable: ptable,
-                              organization: org,
-                              location: loc)
+  let(:os) { FactoryBot.create(:operatingsystem, :with_archs, :with_ptables, type: 'Redhat') }
+  let(:host) do
+    FactoryBot.create(:host, :managed, :with_template_url, operatingsystem: os, ptable: os.ptables.first)
   end
 
   test 'should render template from repository' do
-    assert_not_nil @host.host_params['template_url']
+    assert_not_nil host.params['template_url']
 
     Dir.mktmpdir do |dir|
       kind = 'provision'
 
-      repository_path = "#{dir}/repo.tar.gz"
-      ForemanGitTemplates::Tar.tar(repository_path) do |tar|
-        tar.add_file_simple("templates/#{kind}/whatever.erb", 644, @host.name.length) { |io| io.write(@host.name) }
+      stub_repository host.params['template_url'], "#{dir}/repo.tar.gz" do |tar|
+        tar.add_file_simple("templates/#{kind}/whatever.erb", 644, host.name.length) { |io| io.write(host.name) }
       end
 
-      stub_request(:get, @host.host_params['template_url'])
-        .to_return(status: 200, body: File.new(repository_path))
-
-      get :host_template, params: { kind: kind, spoof: @host.ip }, session: set_session_user
+      get :host_template, params: { kind: kind, spoof: host.ip }, session: set_session_user
       assert_response :success
-      assert_equal @host.name, response.body.strip
+      assert_equal host.name, response.body.strip
     end
   end
 
   test 'should render snippet from repository' do
-    assert_not_nil @host.host_params['template_url']
+    assert_not_nil host.params['template_url']
 
     Dir.mktmpdir do |dir|
       kind = 'PXELinux'
       snippet_name = 'MySnippet'
       snippet_content = 'foo: <%= @foo %>'
       template_content = "<%= snippet('#{snippet_name}', variables: { foo: 'bar' }) %>"
 
-      repository_path = "#{dir}/repo.tar.gz"
-      ForemanGitTemplates::Tar.tar(repository_path) do |tar|
+      stub_repository host.params['template_url'], "#{dir}/repo.tar.gz" do |tar|
         tar.add_file_simple("templates/#{kind}/whatever.erb", 644, template_content.length) { |io| io.write(template_content) }
         tar.add_file_simple("templates/snippet/#{snippet_name}.erb", 644, snippet_content.length) { |io| io.write(snippet_content) }
       end
 
-      stub_request(:get, @host.host_params['template_url'])
-        .to_return(status: 200, body: File.new(repository_path))
-
-      get :host_template, params: { kind: kind, spoof: @host.ip }, session: set_session_user
+      get :host_template, params: { kind: kind, spoof: host.ip }, session: set_session_user
       assert_response :success
       assert_equal 'foo: bar', response.body.strip
     end
   end
 
   test 'snippet should render nested snippet' do
-    assert_not_nil @host.host_params['template_url']
+    assert_not_nil host.params['template_url']
 
     Dir.mktmpdir do |dir|
       nested_snippet_name = 'MyNestedSnippet'
@@ -71,17 +57,13 @@ class UnattendedControllerTest < ActionController::TestCase
       kind = 'PXELinux'
       template_content = "<%= snippet('#{snippet_name}', variables: { foo: 'foo' }) %>"
 
-      repository_path = "#{dir}/repo.tar.gz"
-      ForemanGitTemplates::Tar.tar(repository_path) do |tar|
+      stub_repository host.params['template_url'], "#{dir}/repo.tar.gz" do |tar|
         tar.add_file_simple("templates/#{kind}/whatever.erb", 644, template_content.length) { |io| io.write(template_content) }
         tar.add_file_simple("templates/snippet/#{snippet_name}.erb", 644, snippet_content.length) { |io| io.write(snippet_content) }
         tar.add_file_simple("templates/snippet/#{nested_snippet_name}.erb", 644, nested_snippet_content.length) { |io| io.write(nested_snippet_content) }
       end
 
-      stub_request(:get, @host.host_params['template_url'])
-        .to_return(status: 200, body: File.new(repository_path))
-
-      get :host_template, params: { kind: kind, spoof: @host.ip }, session: set_session_user
+      get :host_template, params: { kind: kind, spoof: host.ip }, session: set_session_user
       assert_response :success
       assert_equal 'foo bar', response.body.strip
     end

test/models/concerns/foreman_git_templates/hostext/operating_system_test.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+require 'test_plugin_helper'
+
+module Hostext
+  class OperatingSystemTest < ActiveSupport::TestCase
+    let(:host) { FactoryBot.create(:host, :managed, :with_template_url) }
+
+    test 'available_template_kinds finds only templates that are defined in the repository' do
+      Dir.mktmpdir do |dir|
+        expected_kinds = ['PXEGrub', 'PXELinux', 'iPXE', 'PXEGrub2', 'provision']
+
+        stub_repository host.params['template_url'], "#{dir}/repo.tar.gz" do |tar|
+          expected_kinds.each do |kind|
+            tar.add_file_simple("templates/#{kind}/whatever.erb", 644, host.name.length) { |io| io.write(host.name) }
+          end
+        end
+
+        actual = host.available_template_kinds('build').map(&:name)
+        assert_same_elements expected_kinds, actual
+      end
+    end
+  end
+end

test/test_plugin_helper.rb

@@ -6,3 +6,16 @@
 # Add plugin to FactoryBot's paths
 FactoryBot.definition_file_paths << File.join(File.dirname(__FILE__), 'factories')
 FactoryBot.reload
+
+def stub_repository(template_url, repository_path)
+  build_repository(repository_path) do |tar|
+    yield tar if block_given?
+  end
+  stub_request(:get, template_url).to_return(status: 200, body: File.new(repository_path))
+end
+
+def build_repository(repository_path)
+  ForemanGitTemplates::Tar.tar(repository_path) do |tar|
+    yield tar if block_given?
+  end
+end

test/unit/foreman/renderer/source/repository_test.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-require 'test_helper'
+require 'test_plugin_helper'
 
 class RepositorySourceTest < ActiveSupport::TestCase
   setup do

test/unit/repository_fetcher_test.rb

@@ -8,7 +8,7 @@ class RepositoryFetcherTest < ActiveSupport::TestCase
       url = 'http://api.com/repository'
       repository_path = "#{dir}/repo.tar.gz"
 
-      ForemanGitTemplates::Tar.tar(repository_path)
+      build_repository repository_path
       stub_request(:get, url).to_return(status: 200, body: repository_path)
       file_path = ForemanGitTemplates::RepositoryFetcher.call(url)
 

test/unit/repository_reader_test.rb

@@ -9,7 +9,7 @@ class RepositoryReaderTest < ActiveSupport::TestCase
       file_name = 'README.md'
       file_content = 'Hello'
 
-      ForemanGitTemplates::Tar.tar(repository_path) do |tar|
+      build_repository repository_path do |tar|
         tar.add_file_simple(file_name, 644, file_content.length) { |io| io.write(file_content) }
       end
 
@@ -26,7 +26,7 @@ class RepositoryReaderTest < ActiveSupport::TestCase
       file_content = 'template'
       another_file_content = 'blah'
 
-      ForemanGitTemplates::Tar.tar(repository_path) do |tar|
+      build_repository repository_path do |tar|
         tar.add_file_simple("#{dir_name}_copy/whatever.erb", 644, another_file_content.length) { |io| io.write(another_file_content) }
         tar.add_file_simple("#{dir_name}/#{file_name}", 644, file_content.length) { |io| io.write(file_content) }
       end
@@ -40,23 +40,37 @@ class RepositoryReaderTest < ActiveSupport::TestCase
     Dir.mktmpdir do |dir|
       repository_path = "#{dir}/repo.tar.gz"
 
-      msg = "Cannot read repository from #{repository_path}"
-      assert_raises_with_message(ForemanGitTemplates::RepositoryReader::RepositoryUnreadableError, msg) do
-        ForemanGitTemplates::RepositoryReader.call(repository_path, 'file.erb')
+      assert_raises(ForemanGitTemplates::RepositoryReader::RepositoryUnreadableError) do
+        ForemanGitTemplates::RepositoryReader.call(repository_path, 'file')
       end
     end
   end
 
-  test 'should raise FileUnreadableError when file does not exist' do
+  test 'should raise MissingFileError when file does not exist' do
     Dir.mktmpdir do |dir|
       repository_path = "#{dir}/repo.tar.gz"
       filename = 'file.erb'
-      ForemanGitTemplates::Tar.tar(repository_path)
+      build_repository repository_path
 
-      msg = "Cannot read #{filename} from repository"
-      assert_raises_with_message(ForemanGitTemplates::RepositoryReader::FileUnreadableError, msg) do
+      assert_raises(ForemanGitTemplates::RepositoryReader::MissingFileError) do
         ForemanGitTemplates::RepositoryReader.call(repository_path, filename)
       end
     end
   end
+
+  test 'should raise EmptyFileError when file is empty' do
+    Dir.mktmpdir do |dir|
+      repository_path = "#{dir}/repo.tar.gz"
+      dir_name = 'provision'
+      file_content = ''
+
+      build_repository repository_path do |tar|
+        tar.add_file_simple("#{dir_name}/whatever.erb", 644, file_content.length) { |io| io.write(file_content) }
+      end
+
+      assert_raises(ForemanGitTemplates::RepositoryReader::EmptyFileError) do
+        ForemanGitTemplates::RepositoryReader.call(repository_path, dir_name)
+      end
+    end
+  end
 end