313 heat files parameter #317

ioggstream · 2017-10-11T16:21:59Z

What does it do?

Resolve files in yaml template.

get_file specs are here https://docs.openstack.org/heat/pike/template_guide/hot_spec.html#get-file

houndci-bot · 2017-10-12T14:58:02Z

test/test_helper.rb

@@ -68,6 +68,14 @@ def array_differences(array_a, array_b)
  (array_a - array_b) | (array_b - array_a)
 end

+def prefix_with_url(files, base_url)
+  return files.map {|fname| File.join(base_url.to_s , fname)}.compact


Redundant return detected.
Space between { and | missing.
Space found before comma.
Space missing inside }.

houndci-bot · 2017-10-12T14:58:03Z

test/requests/orchestration/stack_tests.rb

+        response.body.must_match_schema(@create_format_files)
+        files = response.body['files']
+        Fog::Logger.warning("Request processed: #{files.keys()}")
+        assert_equal_set(["file:///code/test/requests/orchestration/local.yaml", "file:///code/test/requests/orchestration/hot_1.yaml"], files.keys())


Line is too long. [150/120]
Do not use parentheses for method calls with no arguments.

houndci-bot · 2017-10-12T14:58:03Z

test/requests/orchestration/stack_tests.rb

+        response = @orchestration.create_stack(args) 
+        response.body.must_match_schema(@create_format_files)
+        files = response.body['files']
+        Fog::Logger.warning("Request processed: #{files.keys()}")


Do not use parentheses for method calls with no arguments.

houndci-bot · 2017-10-12T14:58:03Z

test/requests/orchestration/stack_tests.rb

+          :stack_name => "teststack_files", 
+          :template   => YAML.load(open("local.yaml")),
+        }
+        response = @orchestration.create_stack(args) 


Trailing whitespace detected.

houndci-bot · 2017-10-12T14:58:03Z

test/requests/orchestration/stack_tests.rb

+      Dir.chdir("/code/test/requests/orchestration") do
+        args = {
+          :stack_name => "teststack_files", 
+          :template   => YAML.load(open("local.yaml")),


Prefer using YAML.safe_load over YAML.load.

houndci-bot · 2017-10-12T14:58:04Z

test/requests/orchestration/stack_files_util_tests.rb

+    it "#recurse_template" do
+      Dir.chdir("test/requests/orchestration") do
+        hot_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(@local_yaml)
+        files = hot_resolver.get_files()


Do not use parentheses for method calls with no arguments.

houndci-bot · 2017-10-12T14:58:04Z

test/requests/orchestration/stack_files_util_tests.rb

+        hot_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(@template_yaml)
+        hot_resolver.get_file_contents(data)
+        Fog::Logger.warning("Processed files: #{hot_resolver.files.keys()}")
+        assert_equal_set(hot_resolver.files.keys(), expected)


Do not use parentheses for method calls with no arguments.

houndci-bot · 2017-10-12T14:58:04Z

test/requests/orchestration/stack_files_util_tests.rb

+      test_cases.each {|data, expected|
+        hot_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(@template_yaml)
+        hot_resolver.get_file_contents(data)
+        Fog::Logger.warning("Processed files: #{hot_resolver.files.keys()}")


Do not use parentheses for method calls with no arguments.

houndci-bot · 2017-10-12T14:58:04Z

test/requests/orchestration/stack_files_util_tests.rb

+      test_cases = @data["get_file_contents_http_template"].map{ |testcase| 
+    #    [ testcase['input'], testcase['expected'] ]
+      }.compact
+      test_cases.each {|data, expected|


Space between { and | missing.
Avoid using {...} for multi-line blocks.

houndci-bot · 2017-10-12T14:58:04Z

test/requests/orchestration/stack_files_util_tests.rb

+
+    it "#get_file_contents_http_template" do
+      test_cases = @data["get_file_contents_http_template"].map{ |testcase| 
+    #    [ testcase['input'], testcase['expected'] ]


Incorrect indentation detected (column 4 instead of 8).

houndci-bot · 2017-10-13T11:19:03Z

test/requests/orchestration/stack_tests.rb

+        response.body.must_match_schema(@create_format_files)
+        files = response.body['files']
+        Fog::Logger.warning("Request processed: #{files.keys}")
+        assert_equal_set(["file:///code/test/requests/orchestration/local.yaml", "file:///code/test/requests/orchestration/hot_1.yaml"], files.keys)


Line is too long. [148/120]

houndci-bot · 2017-10-13T11:19:04Z

test/requests/orchestration/stack_files_util_tests.rb

+      test_cases.each do |data, expected|
+        expected = prefix_with_url(expected, @base_url)
+        hot_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(@template_yaml)
+        hot_resolver.get_file_contents(data, base_url = @base_url.to_s)


Useless assignment to variable - base_url.

houndci-bot · 2017-10-13T11:19:04Z

test/requests/orchestration/stack_files_util_tests.rb

+        [["a", "list"], {}],
+        [{"a" => "dict", "b" => "values"}, {}],
+        [{"type"=>"OS::Nova::Server"}, {}],
+        [{"get_file" => "test/requests/orchestration/foo.sh", "b" => "values"}, {'test/requests/orchestration/foo.sh'=>'# Just a mock'}],


Line is too long. [137/120]

houndci-bot · 2017-10-13T11:19:04Z

test/requests/orchestration/stack_files_util_tests.rb

+    it "#get_file_contents_simple" do
+      test_cases = [
+        ["a string", {}],
+        [["a", "list"], {}],


Use %w or %W for an array of words.

houndci-bot · 2017-10-13T11:19:04Z

test/requests/orchestration/stack_files_util_tests.rb

+      assert_includes(content, "Disallow:")
+    end
+
+    it "#get_content_404" do  # FIXME


Unnecessary spacing detected.

houndci-bot · 2017-10-13T11:19:04Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+              if key == "type" && is_template(file_content) && !(@visited[str_url])
+                if is_object
+                  raise NotImplementedError
+                  template = get_template_contents(


Unreachable code detected.

houndci-bot · 2017-10-13T11:19:04Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+
+              if is_object && object_request
+                raise NotImplementedError
+                file_content = object_request('GET', str_url)


Unreachable code detected.

houndci-bot · 2017-10-13T11:19:05Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+
+          # Actually processing data.
+          if from_data.kind_of?(Hash)
+            from_data.each do |key, value|


Block has too many lines. [34/25]

houndci-bot · 2017-10-13T11:19:05Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          return nil, template
+        end
+
+        def get_file_contents(from_data, base_url = nil, is_object = false, object_request = nil)


Assignment Branch Condition size for get_file_contents is too high. [37.56/15]
Cyclomatic complexity for get_file_contents is too high. [16/6]
Method has too many lines. [49/25]
Perceived complexity for get_file_contents is too high. [19/7]

houndci-bot · 2017-10-13T11:19:05Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+
+          @visited[template_file] = true
+          Fog::Logger.warning("Template visited: #{@visited}")
+          get_file_contents(template, base_url = template_base_url)


Useless assignment to variable - base_url.

houndci-bot · 2017-10-13T13:40:40Z

test/requests/orchestration/stack_files_util_tests.rb

+
+    it "#dont_modify_passed_template" do
+      hot_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(@local_yaml)
+      hot_resolver.get_files()


Do not use parentheses for method calls with no arguments.

houndci-bot · 2017-10-13T13:40:40Z

test/requests/orchestration/stack_files_util_tests.rb

+    Dir.chdir(@oldcwd)
+  end
+
+  describe "success" do


Block has too many lines. [86/25]

houndci-bot · 2017-10-13T13:40:40Z

test/requests/orchestration/stack_files_util_tests.rb

+    @local_yaml = YAML.safe_load(open("local.yaml"))
+    @hot_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(@template_yaml)
+    @base_url = URI.join("file:", File.absolute_path("."))
+    @base_url.host = ""  # fix


Unnecessary spacing detected.

houndci-bot · 2017-10-13T13:40:41Z

test/requests/orchestration/stack_files_util_tests.rb

+require "open-uri"
+require "fog/orchestration/util/recursive_hot_file_loader"  # FIXME: is there a better way to require this file?
+
+describe "Fog::Orchestration[:openstack] | stack requests" do


Block has too many lines. [107/25]

houndci-bot · 2017-10-13T13:40:41Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          return nil, template
+        end
+
+        def get_file_contents(from_data, base_url = nil, is_object = false, object_request = nil)


Assignment Branch Condition size for get_file_contents is too high. [38.79/15]
Cyclomatic complexity for get_file_contents is too high. [17/6]
Method has too many lines. [49/25]
Perceived complexity for get_file_contents is too high. [20/7]

bzwei · 2017-10-16T17:43:56Z

lib/fog/orchestration/openstack/requests/create_stack.rb

+          files = hot_resolver.get_files()
+          if files
+            options['files'] = files
+            options['template'] = hot_resolver.template


Don't mix key :template with 'template'. We should always use symbols in this context.

Why do we need to reassign options[:template]? Here I believe you are actually setting it to a Hash object which is not acceptable by OpenStack.

To ensure unicity of template resources, we need to:

convert every type and get_file reference to a full URI (eg. file:/// or http:///)

On Newton I was able to create the stack referenced in the MIQ ticket using that code, but I still need to write a proper functional testsuite.

bzwei · 2017-10-16T17:52:46Z

lib/fog/orchestration/openstack/requests/create_stack.rb

@@ -28,6 +30,14 @@ def create_stack(arg1, arg2 = nil)
            }.merge(arg2.nil? ? {} : arg2)
          end

+          # Eventually resolve files and update original template.
+          hot_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(options[:template])


OpenStack accepts either options[:template] or options[:template_url]. So we cannot assume options[:template] is always present. Do you want to resolve files for a template from an URL?

Yes, I'll add support for template_url too. I need to add this to https://github.com/fog/fog-openstack/blob/master/docs/orchestration.md as it's not mentioned there.

bzwei · 2017-10-16T18:08:07Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+        end
+
+        def get_files
+          Fog::Logger.warning("Processing template #{@template}")


Why warning? You should lower it to debug level. Should fix all warnings in this PR.

Until in [WIP] I need warnings to show those logs. Then I'll move to debug.

bzwei · 2017-10-16T18:12:05Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+
+        def recurse_if(value)
+          # Should I recurse into this template branch?
+          !!(value.kind_of?(Hash) || value.kind_of?(Array))


!! is redundant.

bzwei · 2017-10-16T21:38:53Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          # XXX Protect from vanilla open-uri attacks
+          uri_or_filename = uri_or_filename[7..-1] if uri_or_filename.start_with?("file:///")
+          open(uri_or_filename) { |f| content = f.read }
+          content == "Error" ? nil : content


This line is problematic. You are not assigning the result to anything. Besides, can content ever be Error?
You made a few comments but did not provide solution. It does not look like final code to be merged.

I've been told to create a [WIP] pull request to simplify code reviews.

For these lines I'd appreciate some help:

which are some common test cases involving open-uri?

do you have any suggestions about securing other parts of this code?

ioggstream · 2017-10-17T07:16:44Z

Hi @bzwei Thank you very much for your time!

Consider this patch is [WIP] and has been pushed to collect code-reviews and comments.

Thanks for all the ruby-related comments! If you have some implementation-related feedback or questions please let me know!

bzwei · 2017-10-17T13:16:40Z

@ioggstream Sorry, I did not realize that it is still wip. Take your time to refine and complete. I will review it more.

ioggstream · 2017-10-17T13:43:51Z

@bzwei don't worry, and thank you very much for all your time! You're welcome to suggest whatever you like so that I can fine-tune the code as soon as possible!

houndci-bot · 2017-10-17T16:44:42Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+            raise NotImplementedError, "template should be Hash or String"
+          end
+
+          get_file_contents(template, base_url = template_base_url)


Useless assignment to variable - base_url.

The way to call the method looks strange. Like the bot says, remove base_url

houndci-bot · 2017-10-17T16:44:42Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+            # Normalize yaml.
+            Fog::Logger.warning("Reingest")
+            template = YAML.safe_load(YAML.dump(template_file))
+            template_base_url = base_url_for_url(normalise_file_path_to_url(Dir.pwd+"/TEMPLATE"))


Surrounding space missing for operator +.

houndci-bot · 2017-10-17T16:44:42Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+              @visited[template_file] = true
+              Fog::Logger.warning("Template visited: #{@visited}")
+            end
+            template = YAML.load(tpl)


Prefer using YAML.safe_load over YAML.load.

@seanhandley As of now, YAML.safe_load has issues with dates. Elsewhere, fog still uses YAML.load

YAML.safe_load("heat_template_version: 2016-10-14") # Psych::DisallowedClass exception (Date)

See ruby/psych#262 (comment)

Try YAML.safe_load(tpl, [Date])

@bzwei ok with YAML.safe_load(tpl, [Date]). It shouldn't break things, right? (eg. deserializes and serializes date in the same format, ...)

houndci-bot · 2017-10-17T16:44:42Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          if template_file.kind_of?(String)
+            if is_template(template_file)
+              tpl = template_file
+              template_base_url = base_url_for_url(normalise_file_path_to_url(Dir.pwd+"/TEMPLATE"))


Surrounding space missing for operator +.

houndci-bot · 2017-10-17T16:44:42Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          content
+        end
+
+        def get_template_contents(template_file, _template_url = nil, _template_object = nil, _existing = false)


Assignment Branch Condition size for get_template_contents is too high. [24.35/15]

Remove the last three arguments if they are not used at all.

houndci-bot · 2017-10-17T16:44:42Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          end
+        end
+
+        def is_template(content)


Rename is_template to template?.

houndci-bot · 2017-10-17T16:44:42Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          Fog::Logger.warning("Processing template #{@template}")
+          _, @template = get_template_contents(@template)
+          Fog::Logger.warning("Template processed. Populated #{@files}")
+          return @files


Redundant return detected.

@seanhandley do I always need to use implicit return in ruby?

houndci-bot · 2017-10-17T16:44:42Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+                                               existing=existing)
+              rescue Exception
+                  # The initial exception gives the user better failure context.
+                  raise template_file_exc


Use 2 (not 4) spaces for indentation.

houndci-bot · 2017-10-17T16:44:43Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+                  return get_template_contents(template_object=template_path,
+                                               object_request=object_request,
+                                               existing=existing)
+              rescue Exception


Avoid rescuing the Exception class. Perhaps you meant to rescue StandardError?

houndci-bot · 2017-10-17T16:44:43Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+              begin
+                  return get_template_contents(template_object=template_path,
+                                               object_request=object_request,
+                                               existing=existing)


Useless assignment to variable - existing. Did you mean _existing?
Surrounding space missing for operator =.

houndci-bot · 2017-10-18T09:15:00Z

test/requests/orchestration/stack_files_util_tests.rb

+      test_cases.each do |data, expected|
+        expected = prefix_with_url(expected, @base_url)
+        hot_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(data)
+        files = hot_resolver.get_files()


Do not use parentheses for method calls with no arguments.

houndci-bot · 2017-10-18T09:15:00Z

test/requests/orchestration/stack_files_util_tests.rb

+      end.compact
+      test_cases.each do |data, expected|
+        hot_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(data)
+        hot_resolver.get_files()


Do not use parentheses for method calls with no arguments.

houndci-bot · 2017-10-18T09:15:00Z

test/requests/orchestration/stack_files_util_tests.rb

+    Dir.chdir(@oldcwd)
+  end
+
+  describe "success" do


Block has too many lines. [89/25]

houndci-bot · 2017-10-18T09:15:00Z

test/requests/orchestration/stack_files_util_tests.rb

+require "open-uri"
+require "fog/orchestration/util/recursive_hot_file_loader"  # FIXME: is there a better way to require this file?
+
+describe "Fog::Orchestration[:openstack] | stack requests" do


Block has too many lines. [110/25]

bzwei · 2017-10-18T13:06:10Z

@ioggstream What if downloading a resource file fails, or a local file does not exist? Should we fail the stack creation immediately because anyway it will fail at the provider side, right?

ioggstream · 2017-10-18T13:30:41Z

@bzwei If a resource file can't be loaded, fog should log and raise an exception. Otherwise, heat will start processing the stack and throw the error in the response.

I'm trying to test the patch inside manageiq and I'm now on both freenode#manageiq and gitter#manageiq. Ping me if you like.

ioggstream · 2017-10-18T15:22:23Z

Update gem in manageiq

  git clone https://github.com/ioggstream/fog-openstack.git
  git checkout remotes/origin/313-heat-files-parameter
  cd /var/www/miq/vmdb
  export PATH+=:$PWD/bin
  bundle exec rake install
  bundle exec rake evm:start

bzwei · 2017-10-18T19:24:10Z

lib/fog/orchestration/openstack/requests/create_stack.rb

+          #  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
+          hot_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url])


suggest file_resolver to better describe the purpose of the variable

Use the resolver only when options[:files] does not exist. In other words, no need to load files if the caller already provides them. Update the comments in https://github.com/fog/fog-openstack/pull/317/files#diff-360fd267b1a87d726bdd369c23976486R10

@bzwei in shade they merge passed and retrieved :files. Without the retrieved ones the stack will fail. https://github.com/openstack-infra/shade/blob/master/shade/openstackcloud.py#L1258

bzwei · 2017-10-18T19:29:33Z

lib/fog/orchestration/openstack/requests/update_stack.rb

+          #  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
+          hot_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url])


Same comment as for create_stack

bzwei · 2017-10-18T19:51:18Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+      #
+      # This implementation just process nested templates but not resource
+      #  registries.
+      class RecursiveHotFileLoader


The logic how to traverse nodes and load files seems over complicated. I have hard time to follow. Can you add comments to each method? We can remove some of them if they turn out to be straightforward.

@bzwei agree but:

my first implementation was simpler

then I preferred to mimic the more stable python implementation

@bzwei Refactored a bit and added comments.

bzwei · 2017-10-18T19:52:19Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          content
+        end
+
+        def get_template_contents(template_file, _template_url = nil, _template_object = nil, _existing = false)


Remove the last three arguments if they are not used at all.

bzwei · 2017-10-18T19:53:47Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+            raise NotImplementedError, "template should be Hash or String"
+          end
+
+          get_file_contents(template, base_url = template_base_url)


The way to call the method looks strange. Like the bot says, remove base_url

bzwei · 2017-10-18T19:56:00Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          elsif template_file.kind_of?(Hash)
+            # Normalize yaml.
+            Fog::Logger.warning("Reingest")
+            template = YAML.safe_load(YAML.dump(template_file))


Why do you need to normalize yaml? Here template_file is already a hash, isn't it?

houndci-bot · 2017-10-19T09:57:16Z

test/requests/orchestration/stack_tests.rb

+      expected = prefix_with_url(["local.yaml", "hot_1.yaml"], @base_url)
+      args = {
+        :stack_name => "teststack_files",
+        :template   => YAML.load(open("local.yaml")),


Prefer using YAML.safe_load over YAML.load.

houndci-bot · 2017-10-19T09:57:16Z

test/requests/orchestration/stack_files_util_tests.rb

+      test_cases.each do |data, expected|
+        expected = prefix_with_url(expected, @base_url)
+        hot_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(@template_yaml)
+        hot_resolver.get_file_contents(data, base_url = @base_url)


Useless assignment to variable - base_url.

houndci-bot · 2017-10-19T09:57:16Z

test/requests/orchestration/stack_files_util_tests.rb

+    Dir.chdir(@oldcwd)
+  end
+
+  describe "success" do


Block has too many lines. [80/25]

houndci-bot · 2017-10-19T09:57:16Z

test/requests/orchestration/stack_files_util_tests.rb

+    @data = @hot_resolver.yaml_load(open("stack_files_util_tests.yaml"))
+    @template_yaml = @hot_resolver.yaml_load(open("template.yaml"))
+    @local_yaml = @hot_resolver.yaml_load(open("local.yaml"))
+


Extra empty line detected at block body end.

houndci-bot · 2017-10-19T12:48:13Z

test/requests/orchestration/stack_files_util_tests.rb

+
+    it "#dont_modify_passed_template" do
+      file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(@local_yaml)
+      file_resolver.get_files()


Do not use parentheses for method calls with no arguments.

houndci-bot · 2017-10-19T12:48:13Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+              # get_file should not recurse hot templates.
+              if key == "type" && template?(file_content) && !(@visited[str_url])
+                template = get_template_contents(
+                  template_url = str_url


Useless assignment to variable - template_url. Did you mean template?

houndci-bot · 2017-10-19T12:48:13Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          return nil, template
+        end
+
+        def get_file_contents(from_data, base_url = nil)


Assignment Branch Condition size for get_file_contents is too high. [31.59/15]
Cyclomatic complexity for get_file_contents is too high. [14/6]
Method has too many lines. [36/25]
Perceived complexity for get_file_contents is too high. [15/7]

houndci-bot · 2017-10-19T12:48:13Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          content
+        end
+
+        def get_template_contents(template_file)


Assignment Branch Condition size for get_template_contents is too high. [20.81/15]

houndci-bot · 2017-10-19T12:48:13Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+        end
+
+        def yaml_load(content)
+          return YAML.load(content) if RUBY_VERSION < "2.1"


Prefer using YAML.safe_load over YAML.load.

houndci-bot · 2017-10-19T12:48:13Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+      #
+      # This implementation just process nested templates but not resource
+      #  registries.
+      class RecursiveHotFileLoader


Class has too many lines. [140/100]

houndci-bot · 2017-10-19T12:48:13Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+#  eg. URI("file:///a.out").to_s != file:///a.out"
+module URI
+  class FILE < Generic
+


Extra empty line detected at class body beginning.

houndci-bot · 2017-10-19T12:48:13Z

lib/fog/orchestration/openstack/requests/update_stack.rb

          else
            # Deprecated, update_stack(stack_id, stack_name, options = {})
            Fog::Logger.deprecation("#update_stack(stack_id, stack_name, options) is deprecated, use #update_stack(stack, options) instead [light_black](#{caller.first})[/]")
            stack_id = arg1
            stack_name = arg2
-            params = {
+            options = {


Useless assignment to variable - options.

ioggstream · 2017-10-19T12:50:54Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+
+# Ruby URI is not round-trip safe when schema == file
+#  eg. URI("file:///a.out").to_s != file:///a.out"
+module URI


@bzwei I need to extend the URI module to fix round-trip safety. Is that fine? Should I put that class elsewhere?

houndci-bot · 2017-10-19T12:59:25Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          return nil, template
+        end
+
+        def get_file_contents(from_data, base_url = nil)


Assignment Branch Condition size for get_file_contents is too high. [31.29/15]
Cyclomatic complexity for get_file_contents is too high. [14/6]
Method has too many lines. [34/25]
Perceived complexity for get_file_contents is too high. [15/7]

houndci-bot · 2017-10-19T12:59:25Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+      #
+      # This implementation just process nested templates but not resource
+      #  registries.
+      class RecursiveHotFileLoader


Class has too many lines. [138/100]

houndci-bot · 2017-10-19T12:59:25Z

lib/fog/orchestration/openstack/requests/update_stack.rb

          else
            # Deprecated, update_stack(stack_id, stack_name, options = {})
            Fog::Logger.deprecation("#update_stack(stack_id, stack_name, options) is deprecated, use #update_stack(stack, options) instead [light_black](#{caller.first})[/]")
            stack_id = arg1
            stack_name = arg2
-            params = {
+            options = {


Useless assignment to variable - options.

houndci-bot · 2017-10-19T12:59:25Z

lib/fog/orchestration/openstack/requests/update_stack.rb

@@ -44,13 +60,13 @@ def update_stack(arg1, arg2 = nil, arg3 = nil)
            stack = arg1
            stack_name = stack.stack_name
            stack_id = stack.id
-            params = arg2.nil? ? {} : arg2
+            options = arg2.nil? ? {} : arg2


Useless assignment to variable - options.

houndci-bot · 2017-10-19T12:59:25Z

lib/fog/orchestration/openstack/requests/update_stack.rb

+          #  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])


Line is too long. [124/120]

houndci-bot · 2017-10-19T12:59:26Z

lib/fog/orchestration/openstack/requests/create_stack.rb

+          end
+
+          if options.key?(:template) || options.key?(:template_url)
+            file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url])


Line is too long. [126/120]

houndci-bot · 2017-10-19T12:59:26Z

lib/fog/orchestration/openstack/requests/create_stack.rb

+          #  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])


Line is too long. [124/120]

houndci-bot · 2017-10-19T12:59:26Z

lib/fog/orchestration/openstack/requests/create_stack.rb

@@ -1,3 +1,5 @@
+require "fog/orchestration/util/recursive_hot_file_loader"  # FIXME is there a better way to require this file?


Unnecessary spacing detected.
Annotation keywords like FIXME should be all upper case, followed by a colon, and a space, then a note describing the problem.

houndci-bot · 2017-10-19T12:59:26Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          return nil, template
+        end
+
+        def get_file_contents(from_data, base_url = nil)


Assignment Branch Condition size for get_file_contents is too high. [31.29/15]
Cyclomatic complexity for get_file_contents is too high. [14/6]
Method has too many lines. [34/25]
Perceived complexity for get_file_contents is too high. [15/7]

houndci-bot · 2017-10-19T12:59:26Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+      #
+      # This implementation just process nested templates but not resource
+      #  registries.
+      class RecursiveHotFileLoader


Class has too many lines. [138/100]

houndci-bot · 2017-10-19T12:59:26Z

lib/fog/orchestration/openstack/requests/update_stack.rb

          else
            # Deprecated, update_stack(stack_id, stack_name, options = {})
            Fog::Logger.deprecation("#update_stack(stack_id, stack_name, options) is deprecated, use #update_stack(stack, options) instead [light_black](#{caller.first})[/]")
            stack_id = arg1
            stack_name = arg2
-            params = {
+            options = {


Useless assignment to variable - options.

houndci-bot · 2017-10-19T12:59:26Z

lib/fog/orchestration/openstack/requests/update_stack.rb

@@ -44,13 +60,13 @@ def update_stack(arg1, arg2 = nil, arg3 = nil)
            stack = arg1
            stack_name = stack.stack_name
            stack_id = stack.id
-            params = arg2.nil? ? {} : arg2
+            options = arg2.nil? ? {} : arg2


Useless assignment to variable - options.

houndci-bot · 2017-10-19T12:59:27Z

lib/fog/orchestration/openstack/requests/update_stack.rb

+          #  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])


Line is too long. [124/120]

houndci-bot · 2017-10-19T12:59:27Z

lib/fog/orchestration/openstack/requests/create_stack.rb

+          end
+
+          if options.key?(:template) || options.key?(:template_url)
+            file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url])


Line is too long. [126/120]

houndci-bot · 2017-10-19T12:59:27Z

lib/fog/orchestration/openstack/requests/create_stack.rb

+          #  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])


Line is too long. [124/120]

houndci-bot · 2017-10-19T12:59:27Z

lib/fog/orchestration/openstack/requests/create_stack.rb

@@ -1,3 +1,5 @@
+require "fog/orchestration/util/recursive_hot_file_loader"  # FIXME is there a better way to require this file?


Unnecessary spacing detected.
Annotation keywords like FIXME should be all upper case, followed by a colon, and a space, then a note describing the problem.

houndci-bot · 2017-10-19T14:18:17Z

test/requests/orchestration/stack_files_util_tests.rb

+    it "#template_file_is_file" do
+      assert(@file_resolver.template_is_url?("local.yaml"))
+      refute(@file_resolver.template_is_url?(YAML.dump(@template_yaml)))
+      refute(@file_resolver.template_is_url?({"a"=>"dict"}))


Redundant curly braces around a hash parameter.

houndci-bot · 2017-10-19T14:18:17Z

test/requests/orchestration/stack_files_util_tests.rb

+    Dir.chdir(@oldcwd)
+  end
+
+  describe "success" do


Block has too many lines. [85/25]

houndci-bot · 2017-10-19T14:18:17Z

test/requests/orchestration/stack_files_util_tests.rb

+require "open-uri"
+require "fog/orchestration/util/recursive_hot_file_loader" # FIXME: is there a better way to require this file?
+
+describe "Fog::Orchestration[:openstack] | stack requests" do


Block has too many lines. [105/25]

houndci-bot · 2017-10-19T14:18:17Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+
+          local_base_url = base_url_for_url(normalise_file_path_to_url(Dir.pwd + "/TEMPLATE"))
+
+          if !(template_file.kind_of?(String) || template_file.kind_of?(Hash))


Favor unless over if for negative conditions.

houndci-bot · 2017-10-19T14:18:17Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          content
+        end
+
+        def get_template_contents(template_file)


Assignment Branch Condition size for get_template_contents is too high. [22.91/15]

houndci-bot · 2017-10-19T14:18:17Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+
+        def template_is_url?(path)
+          # return true if it's an URI, false otherwise.
+          begin


Redundant begin block detected.

houndci-bot · 2017-10-19T14:18:17Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+      #
+      # This implementation just process nested templates but not resource
+      #  registries.
+      class RecursiveHotFileLoader


Class has too many lines. [146/100]

houndci-bot · 2017-10-19T14:18:17Z

lib/fog/orchestration/openstack/requests/update_stack.rb

          else
            # Deprecated, update_stack(stack_id, stack_name, options = {})
            Fog::Logger.deprecation("#update_stack(stack_id, stack_name, options) is deprecated, use #update_stack(stack, options) instead [light_black](#{caller.first})[/]")
            stack_id = arg1
            stack_name = arg2
-            params = {
+            options = {


Useless assignment to variable - options.

houndci-bot · 2017-10-19T14:18:17Z

lib/fog/orchestration/openstack/requests/update_stack.rb

@@ -44,13 +60,13 @@ def update_stack(arg1, arg2 = nil, arg3 = nil)
            stack = arg1
            stack_name = stack.stack_name
            stack_id = stack.id
-            params = arg2.nil? ? {} : arg2
+            options = arg2.nil? ? {} : arg2


Useless assignment to variable - options.

houndci-bot · 2017-10-19T15:31:44Z

test/requests/orchestration/stack_files_util_tests.rb

+    it "#get_file_contents_http_template" do
+      skip if Fog.mocking?
+      test_cases = @data["get_file_contents_http_template"].map do |testcase|
+        [ testcase['input'], testcase['expected'] ]


Space inside square brackets detected.

houndci-bot · 2017-10-19T15:31:45Z

test/requests/orchestration/stack_files_util_tests.rb

+      #  values are absolute paths uri and should be resolved with the local
+      #  directory.
+      test_cases = @data['get_file_contents_local_template'].map do |testcase|
+        [ testcase['input'], testcase['expected'] ]


Space inside square brackets detected.

houndci-bot · 2017-10-19T15:31:45Z

test/requests/orchestration/stack_files_util_tests.rb

+  end
+
+  describe "success" do
+


Extra empty line detected at block body beginning.

houndci-bot · 2017-10-19T15:31:45Z

test/requests/orchestration/stack_files_util_tests.rb

+    Dir.chdir(@oldcwd)
+  end
+
+  describe "success" do


Block has too many lines. [58/25]

houndci-bot · 2017-10-19T15:31:45Z

test/requests/orchestration/stack_files_util_tests.rb

+require "open-uri"
+require "fog/orchestration/util/recursive_hot_file_loader" # FIXME: is there a better way to require this file?
+
+describe "Fog::Orchestration[:openstack] | stack requests" do


Block has too many lines. [78/25]

houndci-bot · 2017-10-19T15:31:45Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          return nil, template
+        end
+
+        def get_file_contents(from_data, base_url = nil)


Assignment Branch Condition size for get_file_contents is too high. [27.89/15]
Cyclomatic complexity for get_file_contents is too high. [12/6]
Perceived complexity for get_file_contents is too high. [12/7]

houndci-bot · 2017-10-19T15:31:45Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          content
+        end
+
+        def get_template_contents(template_file)


Assignment Branch Condition size for get_template_contents is too high. [22.05/15]

houndci-bot · 2017-10-19T15:31:45Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+      #
+      # This implementation just process nested templates but not resource
+      #  registries.
+      class RecursiveHotFileLoader


Class has too many lines. [141/100]

houndci-bot · 2017-10-19T15:31:45Z

lib/fog/orchestration/openstack/requests/update_stack.rb

          else
            # Deprecated, update_stack(stack_id, stack_name, options = {})
            Fog::Logger.deprecation("#update_stack(stack_id, stack_name, options) is deprecated, use #update_stack(stack, options) instead [light_black](#{caller.first})[/]")
            stack_id = arg1
            stack_name = arg2
-            params = {
+            options = {


Useless assignment to variable - options.

houndci-bot · 2017-10-19T15:31:45Z

lib/fog/orchestration/openstack/requests/update_stack.rb

@@ -44,13 +60,13 @@ def update_stack(arg1, arg2 = nil, arg3 = nil)
            stack = arg1
            stack_name = stack.stack_name
            stack_id = stack.id
-            params = arg2.nil? ? {} : arg2
+            options = arg2.nil? ? {} : arg2


Useless assignment to variable - options.

houndci-bot · 2017-10-19T15:50:43Z

test/requests/orchestration/stack_files_util_tests.rb

+    Dir.chdir(@oldcwd)
+  end
+
+  describe "success" do


Block has too many lines. [56/25]

houndci-bot · 2017-10-19T15:50:43Z

test/requests/orchestration/stack_files_util_tests.rb

+require "open-uri"
+require "fog/orchestration/util/recursive_hot_file_loader" # FIXME: is there a better way to require this file?
+
+describe "Fog::Orchestration[:openstack] | stack requests" do


Block has too many lines. [76/25]

houndci-bot · 2017-10-19T15:50:44Z

test/requests/orchestration/stack_files_util_helper_tests.rb

+    Dir.chdir(@oldcwd)
+  end
+
+  describe "success" do


Block has too many lines. [35/25]

houndci-bot · 2017-10-19T15:50:44Z

test/requests/orchestration/stack_files_util_helper_tests.rb

+require "open-uri"
+require "fog/orchestration/util/recursive_hot_file_loader" # FIXME: is there a better way to require this file?
+
+describe "Fog::Orchestration[:openstack] | stack requests" do


Block has too many lines. [55/25]

houndci-bot · 2017-10-20T14:49:15Z

test/requests/orchestration/stack_files_util_tests.rb

+    it "#get_file_contents_simple" do
+      test_cases = [
+        ["a string", {}],
+        [["a", "list"], {}],


Use %w or %W for an array of words.

houndci-bot · 2017-10-20T14:49:15Z

test/requests/orchestration/stack_tests.rb

+      expected = prefix_with_url(["local.yaml", "hot_1.yaml", "file.txt"], @base_url)
+      args = {
+        :stack_name => "teststack_files",
+        :template   => YAML.load(open("local.yaml")),


Prefer using YAML.safe_load over YAML.load.

houndci-bot · 2017-10-20T14:49:15Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          content
+        end
+
+        def get_template_contents(template_file)


Assignment Branch Condition size for get_template_contents is too high. [22.05/15]

houndci-bot · 2017-10-20T14:49:15Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+      #
+      # This implementation just process nested templates but not resource
+      #  registries.
+      class RecursiveHotFileLoader


Class has too many lines. [134/100]

houndci-bot · 2017-10-20T14:49:15Z

lib/fog/orchestration/openstack/requests/update_stack.rb

+          end
+
+          if options.key?(:template) || options.key?(:template_url)
+            file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url], options[:files])


Line is too long. [143/120]

houndci-bot · 2017-10-20T14:49:15Z

lib/fog/orchestration/openstack/requests/update_stack.rb

+          #  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])


Line is too long. [141/120]

houndci-bot · 2017-10-20T14:49:15Z

lib/fog/orchestration/openstack/requests/create_stack.rb

+          end
+
+          if options.key?(:template) || options.key?(:template_url)
+            file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url], options[:files])


Line is too long. [143/120]

houndci-bot · 2017-10-20T14:49:15Z

lib/fog/orchestration/openstack/requests/create_stack.rb

+          #  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])


Line is too long. [141/120]

ioggstream · 2017-10-20T15:11:17Z

@bzwei Here is the functional test. There's a commented script on how to run this.
https://gist.github.com/ioggstream/545e39c13e3a922405326af4f2b211cb

ioggstream · 2017-10-20T15:44:29Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          # throw exceptions enables stack creation to fail
+          #   with a suitable error.
+          #
+          # XXX Implement a retry here?


This first implementation won't contain a retry. It could be added in the future.

houndci-bot · 2017-11-02T14:01:19Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+      end # Class
+    end
+  end
+end


Final newline missing.

houndci-bot · 2017-11-02T14:01:19Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+
+          YAML.safe_load(YAML.dump(item), [Date])
+        end
+      end # Class


Do not place comments on the same line as the end keyword.

houndci-bot · 2017-11-02T14:01:20Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+        #   replaced with their absolute URI as done in heatclient
+        #   and shade.
+        #
+        def get_file_contents(from_data, base_url = nil)


Assignment Branch Condition size for get_file_contents is too high. [20.9/15]
Cyclomatic complexity for get_file_contents is too high. [9/6]
Perceived complexity for get_file_contents is too high. [9/7]

houndci-bot · 2017-11-02T14:01:20Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+      #
+      # This implementation just process nested templates but not resource
+      #  registries.
+      class RecursiveHotFileLoader


Class has too many lines. [119/100]

bzwei · 2017-11-02T18:19:11Z

lib/fog/orchestration/openstack/requests/create_stack.rb

+          #  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])


better this way

if options[:files].nil? file_resolver = Fog::Orchestration::Util::RecursiveFileLoader.new(options[:template] || options[:template_url]) files = file_resolver.get_files options[:files] = files unless files.empty? options[:template] = file_resolver.template end

:files reintroduced after gitter discussion

bzwei · 2017-11-02T18:20:08Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+      class RecursiveHotFileLoader
+        attr_reader :files
+        attr_reader :template
+        attr_reader :visited


delete, no need to expose it as read_only attribute

visited removed

template is used to return the processed template with URIs

bzwei · 2017-11-02T18:23:51Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+        attr_reader :template
+        attr_reader :visited
+
+        def initialize(template, files = nil)


def initialize(template)

Reintroduced after gitter discussion

bzwei · 2017-11-02T18:25:07Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          # 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.
+          @template = deep_copy(template)


No need to deep_copy. Why do we expect template can be a Hash? Initially it is either the content string or url.

According to https://github.com/fog/fog-openstack/blob/master/docs/orchestration.md#stacks
stack_update accepts Hash too.
https://github.com/fog/fog-openstack/blame/master/docs/orchestration.md#L122

# from the above link. heat_template = { "template": { "description": "Updated description" } } stack.save(heat_template)

This allows the code to work in both cases.

Ok, we agreed on irc to leave the deepcopy and support Hash to support orchestration.md examples.

bzwei · 2017-11-02T18:25:13Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+        end
+
+        def get_files
+          return @files unless @files.empty?


return @files unless @files.nil? # avoid processing templates for every `get_files` call @files = {}

@bzwei agreed on gitter that we'll reintroduce files argument.

bzwei · 2017-11-02T18:29:24Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          #  avoid modifying the argument. Instead create a
+          #  new one to be modified by get_file_contents.
+          @template = deep_copy(template)
+          @files = files || {}


@bzwei reintroduced after gitter discussion.

bzwei · 2017-11-02T18:35:11Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          @visited = Set.new
+        end
+
+        def get_files


get_files method is not needed if we write initialize this way

def initialize(template) @files = {} @visited = Set.new @template = get_template_content(template) end

In the caller you can simply

file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url] options[:template] = file_resolver.template options[:files] = file_resolver.files unless file_resolver.files.empty?

…late, :files.

houndci-bot · 2017-11-04T14:18:03Z

test/requests/orchestration/stack_files_util_tests.rb

+      end.compact
+      test_cases.each do |data, _|
+        assert_raises ArgumentError, URI::InvalidURIError do
+          file_resolver = Fog::Orchestration::Util::RecursiveHotFileLoader.new(data)


Useless assignment to variable - file_resolver.

houndci-bot · 2017-11-04T14:18:03Z

test/requests/orchestration/stack_files_util_tests.rb

+    Dir.chdir(@oldcwd)
+  end
+
+  describe "success" do


Block has too many lines. [69/25]

houndci-bot · 2017-11-04T14:18:03Z

test/requests/orchestration/stack_files_util_tests.rb

+require "open-uri"
+require "fog/orchestration/util/recursive_hot_file_loader"
+
+describe "Fog::Orchestration[:openstack] | stack requests" do


Block has too many lines. [89/25]

houndci-bot · 2017-11-04T14:18:03Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+        #
+        # XXX: we could use named parameters
+        # and better mimic heatclient implementation.
+        def get_template_contents(template_file)


Assignment Branch Condition size for get_template_contents is too high. [23.39/15]

houndci-bot · 2017-11-04T14:18:03Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+      #
+      # This implementation just process nested templates but not resource
+      #  registries.
+      class RecursiveHotFileLoader


Class has too many lines. [114/100]

…late, :files.

houndci-bot · 2017-11-04T14:25:27Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+      #
+      # This implementation just process nested templates but not resource
+      #  registries.
+      class RecursiveHotFileLoader


Class has too many lines. [112/100]

bzwei · 2017-11-07T14:09:54Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+        #   replaced with their absolute URI as done in heatclient
+        #   and shade.
+        #
+        def get_file_contents(from_data, base_url = nil)


Can base_url ever be nil? You always have a base_url when calling from get_template_contents. We don't need a default value here.

bzwei · 2017-11-07T14:18:06Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+          Fog::Logger.debug("Processing #{from_data} with base_url #{base_url}")
+
+          # Recursively traverse the tree.
+          if recurse_if(from_data)


We don't need a recuse_if method because you need to test again whether from_data is a Hash.
Consider

recurse_data = from_data.kind_of?(Hash) ? from_data.values : from_data recurse_data.each { |value| get_file_contents(value, base_url) } if recurse_data.kind_of?(Array)

I think it is more flexible/readable having a single function to decide, but your implementation is so clever :D

I changed it and added a small comment to make it more straightforward.

bzwei · 2017-11-07T14:32:21Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+            raise ArgumentError if url.host.nil? && remote_schemes.include?(url.scheme)
+
+            # Encode URI with spaces.
+            uri_or_filename = URI.encode(URI.decode(URI(uri_or_filename).to_s))


Do we need to address the obsolete warning?
you already have url, so the line could be url_or_filename = URI.encode(URI.decode(url.to_s))

1- I'm finding a suitable replacement for URI.decode. Hints welcome.

2- Applied your suggestion and removed the 3rd URI call

Replaced with gsub(/ /, "%20")

bzwei · 2017-11-07T14:56:55Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+        end
+
+        def deep_copy(item)
+          return item if item.kind_of?(String)


better

return item unless item.kind_of?(Hash)

while imho:

that won't deep_copy arrays

may result counter-intuitive during further development

I'm fine for changing if it's a merge-blocker ;)

houndci-bot · 2017-11-07T16:55:51Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+        #   replaced with their absolute URI as done in heatclient
+        #   and shade.
+        #
+        def get_file_contents(from_data, base_url)


Assignment Branch Condition size for get_file_contents is too high. [20.9/15]
Cyclomatic complexity for get_file_contents is too high. [9/6]
Perceived complexity for get_file_contents is too high. [9/7]

houndci-bot · 2017-11-07T16:55:51Z

lib/fog/orchestration/util/recursive_hot_file_loader.rb

+      #
+      # This implementation just process nested templates but not resource
+      #  registries.
+      class RecursiveHotFileLoader


Class has too many lines. [107/100]

bzwei · 2017-11-07T18:16:14Z

lib/fog/orchestration/openstack/requests/create_stack.rb

@@ -1,3 +1,5 @@
+require "fog/orchestration/util/recursive_hot_file_loader"


You can add the package in https://github.com/fog/fog-openstack/blob/master/lib/fog/openstack.rb then you don't need to require here and nor need to use the full namespace while using the resolver class.

Rock! Thank you!!!

houndci-bot · 2017-11-08T09:12:59Z

test/requests/orchestration/stack_files_util_tests.rb

+require "yaml"
+require "open-uri"
+
+describe "Fog::Orchestration[:openstack] | stack requests" do


Block has too many lines. [89/25]

houndci-bot · 2017-11-08T09:13:00Z

test/requests/orchestration/stack_files_util_helper_tests.rb

+require "yaml"
+require "open-uri"
+
+describe "Fog::Orchestration[:openstack] | stack requests" do


Block has too many lines. [65/25]

houndci-bot · 2017-11-08T09:13:00Z

lib/fog/orchestration/openstack/requests/update_stack.rb

+          end
+
+          if options.key?(:template) || options.key?(:template_url)
+            file_resolver = Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url], options[:files])


Line is too long. [123/120]

houndci-bot · 2017-11-08T09:13:00Z

lib/fog/orchestration/openstack/requests/update_stack.rb

+          #  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 = Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url], options[:files])


Line is too long. [121/120]

houndci-bot · 2017-11-08T09:13:00Z

lib/fog/orchestration/openstack/requests/create_stack.rb

+          end
+
+          if options.key?(:template) || options.key?(:template_url)
+            file_resolver = Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url], options[:files])


Line is too long. [123/120]

houndci-bot · 2017-11-08T09:13:00Z

lib/fog/orchestration/openstack/requests/create_stack.rb

+          #  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 = Util::RecursiveHotFileLoader.new(options[:template] || options[:template_url], options[:files])


Line is too long. [121/120]

bzwei · 2017-11-08T14:13:13Z

@ioggstream Thanks!

ioggstream · 2017-11-08T14:18:10Z

@bzwei Thanks to you!

The implementation is based on the python-heatclient one. Where needed, test data is retrieved from a yaml file. This code allows creating a first simple stack referencing get_file: resources.

Ruby URI module uses minimal file representation (eg. file:/etc/hosts) instead of the traditional one (eg. file:///etc/hosts). Created functions to mimic Heat URI behavior (file:///).

* added Heat spec references in comments; * removed redundant traversal; * moved log.warnings to log.debug in template traversal; * removed 1.9 support code;

* Simplified local path normalization. * Rename get_content to read_uri

When a Hash template is passed, it shouldn't be modified in-place. When a files parameter is passed, the files referenced by existing keys should be skipped, the missing ones should be added in-place. This enables overriding discovered files location.

…ecated URI methods. Files are now resolved - downloaded and parsed - at instance initialization. It is not necessary to call the resolver method anymore. This enabled removing some methods. This commit removes the deprecaded URI.encode and URI.decode methods.

Ladas

Thanks @bzwei for a detail review. I've browsed this couple of times and it looks acceptable.

Ladas

Thanks @bzwei for a detail review. I've browsed this couple of times and it looks acceptable.

Ladas

Thanks @bzwei for a detail review. I've browsed this couple of times and it looks acceptable.

Ladas · 2017-11-13T07:56:48Z

eh, github acts funky

houndci-bot reviewed Oct 12, 2017

View reviewed changes

ioggstream force-pushed the 313-heat-files-parameter branch from 24cd567 to e81e8f4 Compare October 12, 2017 17:00

houndci-bot reviewed Oct 13, 2017

View reviewed changes

ioggstream force-pushed the 313-heat-files-parameter branch from 0101499 to 50b276f Compare October 13, 2017 11:23

ioggstream mentioned this pull request Oct 13, 2017

heat client don't populate correctly the "files" parameter ManageIQ/manageiq#15563

Closed

houndci-bot reviewed Oct 13, 2017

View reviewed changes

ioggstream mentioned this pull request Oct 13, 2017

#322 Conform variable names to create_stack and signature. #323

Merged

bzwei reviewed Oct 16, 2017

View reviewed changes

ioggstream force-pushed the 313-heat-files-parameter branch from 8ccf5ae to 233f79e Compare October 17, 2017 16:44

houndci-bot reviewed Oct 17, 2017

View reviewed changes

houndci-bot reviewed Oct 18, 2017

View reviewed changes

bzwei reviewed Oct 18, 2017

View reviewed changes

houndci-bot reviewed Oct 19, 2017

View reviewed changes

ioggstream commented Oct 19, 2017

View reviewed changes

houndci-bot reviewed Oct 19, 2017

View reviewed changes

ioggstream force-pushed the 313-heat-files-parameter branch from 0c58eea to da19385 Compare October 19, 2017 15:50

houndci-bot reviewed Oct 19, 2017

View reviewed changes

houndci-bot reviewed Oct 20, 2017

View reviewed changes

ioggstream commented Oct 20, 2017

View reviewed changes

ioggstream added a commit to ioggstream/fog-openstack that referenced this pull request Nov 2, 2017

Fixes fog#317 (comment): only deep_copy templates in the constructor.

bfa5712

houndci-bot reviewed Nov 2, 2017

View reviewed changes

bzwei reviewed Nov 2, 2017

View reviewed changes

ioggstream added a commit to ioggstream/fog-openstack that referenced this pull request Nov 4, 2017

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

dc500f8

…late, :files.

houndci-bot reviewed Nov 4, 2017

View reviewed changes

ioggstream force-pushed the 313-heat-files-parameter branch from dc500f8 to 908fe22 Compare November 4, 2017 14:25

ioggstream added a commit to ioggstream/fog-openstack that referenced this pull request Nov 4, 2017

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

908fe22

…late, :files.

houndci-bot reviewed Nov 4, 2017

View reviewed changes

bzwei reviewed Nov 7, 2017

View reviewed changes

houndci-bot reviewed Nov 7, 2017

View reviewed changes

ioggstream changed the title ~~[WIP] 313 heat files parameter~~ 313 heat files parameter Nov 7, 2017

bzwei reviewed Nov 7, 2017

View reviewed changes

ioggstream added a commit to ioggstream/fog-openstack that referenced this pull request Nov 8, 2017

Fixes: fog#317 (review) Lazy load of RecursiveHotFileLoader.

b1393ad

houndci-bot reviewed Nov 8, 2017

View reviewed changes

bzwei approved these changes Nov 8, 2017

View reviewed changes

ioggstream added 6 commits November 10, 2017 17:49

Heat: first running implementation supporting files parameter.

ee28994

The implementation is based on the python-heatclient one. Where needed, test data is retrieved from a yaml file. This code allows creating a first simple stack referencing get_file: resources.

Work around Ruby URI behavior.

64cdb1d

Ruby URI module uses minimal file representation (eg. file:/etc/hosts) instead of the traditional one (eg. file:///etc/hosts). Created functions to mimic Heat URI behavior (file:///).

Reference Heat specs. Code cleanup.

deecbfc

* added Heat spec references in comments; * removed redundant traversal; * moved log.warnings to log.debug in template traversal; * removed 1.9 support code;

Refactored redundant uri/path code. Added comments.

2b32ce8

* Simplified local path normalization. * Rename get_content to read_uri

ioggstream force-pushed the 313-heat-files-parameter branch from b1393ad to ef26536 Compare November 10, 2017 17:20

Ladas approved these changes Nov 13, 2017

View reviewed changes

Ladas merged commit bd69c6f into fog:master Nov 13, 2017

ioggstream mentioned this pull request Nov 13, 2017

Support heat files merge #337

Closed

		@@ -1,3 +1,5 @@
		require "fog/orchestration/util/recursive_hot_file_loader" # FIXME is there a better way to require this file?


		local_base_url = base_url_for_url(normalise_file_path_to_url(Dir.pwd + "/TEMPLATE"))

		if !(template_file.kind_of?(String) \|\| template_file.kind_of?(Hash))

313 heat files parameter #317

313 heat files parameter #317

Conversation

ioggstream commented Oct 11, 2017 • edited Loading

What does it do?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioggstream commented Oct 17, 2017

bzwei commented Oct 17, 2017

ioggstream commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzwei commented Oct 18, 2017

ioggstream commented Oct 18, 2017

ioggstream commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioggstream Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioggstream Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioggstream commented Oct 11, 2017 •

edited

Loading

ioggstream Oct 19, 2017 •

edited

Loading

ioggstream Oct 19, 2017 •

edited

Loading