Fixes fog#317 (comment) Remove get_files and retain attr_reader :temp…

…late, :files.
ioggstream · Nov 4, 2017 · 908fe22 · 908fe22
1 parent 3239bd8
commit 908fe22
Show file tree

Hide file tree

Showing 5 changed files with 25 additions and 42 deletions.
diff --git a/lib/fog/orchestration/openstack/requests/create_stack.rb b/lib/fog/orchestration/openstack/requests/create_stack.rb
@@ -34,15 +34,14 @@ def create_stack(arg1, arg2 = nil)
           # Templates should always:
           #  - be strings
           #  - contain URI references instead of relative paths.
-          # Passing :template_url may not work well with `get_files` and remote `type`:
+          # Passing :template_url may not work well with `get_file` and remote `type`:
           #  the python client implementation in shade retrieves from :template_uri
           #  and replaces it with :template.
           #  see https://github.com/openstack-infra/shade/blob/master/shade/openstackcloud.py#L1201
           #  see https://developer.openstack.org/api-ref/orchestration/v1/index.html#create-stack
           file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url], options[:files])
-          files = file_resolver.get_files
           options[:template] = file_resolver.template
-          options[:files] = files if files
+          options[:files] = file_resolver.files unless file_resolver.files.empty?
 
           request(
             :expects => 201,
@@ -91,8 +90,7 @@ def create_stack(arg1, arg2 = nil)
 
           if options.key?(:template) || options.key?(:template_url)
             file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url], options[:files])
-            files = file_resolver.get_files
-            response.body['files'] = files if files
+            response.body['files'] = file_resolver.files unless file_resolver.files.empty?
           end
 
           response

diff --git a/lib/fog/orchestration/openstack/requests/update_stack.rb b/lib/fog/orchestration/openstack/requests/update_stack.rb
@@ -34,15 +34,14 @@ def update_stack(arg1, arg2 = nil, arg3 = nil)
           # Templates should always:
           #  - be strings
           #  - contain URI references instead of relative paths.
-          # Passing :template_url may not work well with `get_files` and remote `type`:
+          # Passing :template_url may not work well with `get_file` and remote `type`:
           #  the python client implementation in shade retrieves from :template_uri
           #  and replaces it with :template.
           #  see https://github.com/openstack-infra/shade/blob/master/shade/openstackcloud.py#L1201
           #  see https://developer.openstack.org/api-ref/orchestration/v1/index.html#create-stack
           file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url], options[:files])
-          files = file_resolver.get_files
           options[:template] = file_resolver.template
-          options[:files] = files if files
+          options[:files] = file_resolver.files unless file_resolver.files.empty?
 
           request(
             :expects => 202,
@@ -77,8 +76,7 @@ def update_stack(arg1, arg2 = nil, arg3 = nil)
 
           if options.key?(:template) || options.key?(:template_url)
             file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url], options[:files])
-            files = file_resolver.get_files
-            response.body['files'] = files if files
+            response.body['files'] = file_resolver.files unless file_resolver.files.empty?
           end
 
           response = Excon::Response.new

diff --git a/lib/fog/orchestration/util/recursive_hot_file_loader.rb b/lib/fog/orchestration/util/recursive_hot_file_loader.rb
@@ -23,22 +23,14 @@ class RecursiveHotFileLoader
         attr_reader :template
 
         def initialize(template, files = nil)
-          # Serialize the template hash to deep-copy it and
-          #  avoid modifying the argument. Instead create a
-          #  new one to be modified by get_file_contents.
-          #
           # According to https://github.com/fog/fog-openstack/blame/master/docs/orchestration.md#L122
           #  templates can be either String or Hash.
-          @template = deep_copy(template)
-          @files = files || {}
+          #  If it's an Hash, we deep_copy it so the passed argument
+          #  is not modified by get_file_contents.
+          template = deep_copy(template)
           @visited = Set.new
-        end
-
-        def get_files
-          Fog::Logger.debug("Processing template #{@template}")
-          @template = get_template_contents(@template)
-          Fog::Logger.debug("Template processed. Populated #{@files}")
-          @files
+          @files = files || {}
+          @template = get_template_contents(template)
         end
 
         # Return string
@@ -64,9 +56,9 @@ def url_join(prefix, suffix)
         # XXX: we could use named parameters
         # and better mimic heatclient implementation.
         def get_template_contents(template_file)
-          Fog::Logger.debug("get_template_contents #{template_file}")
+          Fog::Logger.debug("get_template_contents [#{template_file}]")
 
-          raise "template_file should be Hash or String" unless
+          raise "template_file should be Hash or String, not #{template_file.class.name}" unless
             template_file.kind_of?(String) || template_file.kind_of?(Hash)
 
           local_base_url = url_join("file:/", File.absolute_path(Dir.pwd))
@@ -232,4 +224,4 @@ def deep_copy(item)
       end
     end
   end
-end
+end
diff --git a/test/requests/orchestration/stack_files_util_helper_tests.rb b/test/requests/orchestration/stack_files_util_helper_tests.rb
@@ -12,12 +12,12 @@
   before do
     @oldcwd = Dir.pwd
     Dir.chdir("test/requests/orchestration")
-    @orchestration = Fog::Orchestration[:openstack]
-    @file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(@template_yaml)
     @base_url = "file://" + File.absolute_path(".")
-    @data =YAML.load_file("stack_files_util_tests.yaml")
-    @template_yaml =YAML.load_file("template.yaml")
-    @local_yaml =YAML.load_file("local.yaml")
+    @data = YAML.load_file("stack_files_util_tests.yaml")
+    @template_yaml = YAML.load_file("template.yaml")
+    @local_yaml = YAML.load_file("local.yaml")
+    @orchestration = Fog::Orchestration[:openstack]
+    @file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new({})
   end
   after do
     Dir.chdir(@oldcwd)

diff --git a/test/requests/orchestration/stack_files_util_tests.rb b/test/requests/orchestration/stack_files_util_tests.rb
@@ -12,12 +12,12 @@
   before do
     @oldcwd = Dir.pwd
     Dir.chdir("test/requests/orchestration")
-    @orchestration = Fog::Orchestration[:openstack]
-    @file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(@template_yaml)
     @base_url = "file://" + File.absolute_path(".")
     @data = YAML.load_file("stack_files_util_tests.yaml")
     @template_yaml = YAML.load_file("template.yaml")
     @local_yaml = YAML.load_file("local.yaml")
+    @orchestration = Fog::Orchestration[:openstack]
+    @file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new({})
   end
   after do
     Dir.chdir(@oldcwd)
@@ -33,7 +33,7 @@
         [{"get_file" => "foo.sh", "b" => "values"}, {'foo.sh'=>'# Just a mock'}],
       ]
       test_cases.each do |data, expected|
-        file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(@template_yaml)
+        file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new({})
         file_resolver.send(:get_file_contents, data)
         assert_equal(file_resolver.files, expected)
       end
@@ -49,7 +49,7 @@
       test_cases.each do |data, expected|
         Fog::Logger.warning("Testing with #{data} #{expected}")
         expected = prefix_with_url(expected, @base_url)
-        file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(@template_yaml)
+        file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new({})
         file_resolver.send(:get_file_contents, data, @base_url)
         assert_equal(file_resolver.files.keys, expected)
       end
@@ -60,10 +60,8 @@
         [testcase['input'], testcase['expected']]
       end.compact
       test_cases.each do |data, _|
-        file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(data)
-
         assert_raises ArgumentError, URI::InvalidURIError do
-          file_resolver.get_files
+          file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(data)
         end
       end
     end
@@ -75,7 +73,6 @@
       end.compact
       test_cases.each do |data, expected|
         file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(data)
-        file_resolver.get_files
         assert_equal_set(file_resolver.files.keys, expected)
       end
     end
@@ -88,14 +85,12 @@
       test_cases.each do |data, expected|
         expected = prefix_with_url(expected, @base_url)
         file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(data)
-        files = file_resolver.get_files
-        assert_equal_set(files.keys, expected)
+        assert_equal_set(file_resolver.files.keys, expected)
       end
     end
 
     it "#dont_modify_passed_template" do
       file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(@local_yaml)
-      file_resolver.get_files
       template = file_resolver.template
 
       # The template argument should be modified.