Allow users to reference resources from dependencies #1080

stevendanna · 2016-09-15T07:56:04Z

All resources from deps are added into the control_eval_context used by
the current profile. However, if there is a name conflict, the last
loaded resource wins. The new require_resource dsl method allows the
user to do the following:

require_resource(profile: 'profile_name',
                 resource: 'other',
                as: 'renamed')

describe renamed do
  ...
end

Signed-off-by: Steven Danna steve@chef.io

stevendanna · 2016-09-15T08:33:28Z

lib/inspec/runner.rb

@@ -182,58 +182,10 @@ def register_rules(ctx)

    private

-    def block_source_info(block)


This just moved over to the rspec_formatter because I think it makes more sense there. If we want I can do that in a different pull request.

be aware that inspec json should return the same information. Therefore I disagree with the move to the rspec formatter

Minor correction on my original comment: I move this to the rspec_runner not the rspec formatter.

I'm not sure if that difference influences your comment. My thinking was that the only callsite of this function seems to be called in check_to_example (did I miss one?) whose purpose seems tied precisely to the rspec_runner, so I moved both functions there.

Not sure why we move this to the RSpecRunner. the runner is the right place. By moving this to the RSpec runner, this method is not available to the mock runner anymore.

I'll move it back. The general reasons why I moved it was because with it here it makes are distinction between rspec runner a bit academic since this is tied directly to the rspec runner. But this isn't particularly a hill I care about.

chris-rock · 2016-09-15T11:42:39Z

lib/inspec/runner.rb

@@ -182,58 +182,10 @@ def register_rules(ctx)

    private

-    def block_source_info(block)


be aware that inspec json should return the same information. Therefore I disagree with the move to the rspec formatter

chris-rock · 2016-09-15T11:44:01Z

lib/inspec/plugins/resource.rb

@@ -72,6 +72,9 @@ def inspec
      end

      # rubocop:enable Lint/NestedMethodDefinition
+      if __resource_registry.key?(name)
+        Inspec::Log.warn("Overwriting resource #{name}. To reference a specific version of #{name} use the resource() method")


That is a nice warning

stevendanna · 2016-09-15T14:26:27Z

@arlimus We had a discussion about possible syntaxes for this feature.

Default:

 gordon_config(args)

Will continue to work, gives access to the last eval'd resource with that name

Option 1:

some_profile.gordon_config(args)

Pros

Feels nice to type, minimal extra characters

Cons

not all names safe as barewords - / .
profile and resources that have the same name would still conflict

Option 2:

resource('profile_d', 'gordon_config', args)

Cons

looks different than normal resource calls

Pros

Straightforward implementation (see PR)
Very explicit

Option 2b:

resource(profile: 'foobar', resource: 'gordon_config', args: ['foo', 'bar'])

Pros

Extensible
Can be implemented on top of 2 later

Cons

Passing args is a bit cumbersome

Option 3:

resource_name = resource(profile_name, resource_name)
resource_name(args)

With the potential shortcut (if we can implement it):

resource(profile_name, resource_name)

Cons

Might need some Ruby Magic(TM) to implement

Pros

Allows you to locally rebind resources to a new name

Option 4

profile('profilename').resource_name(args)

Pros

profile is possibly good to hang other functions off of in the future

Cons

YAGNI?

stevendanna · 2016-09-15T14:26:45Z

My personal vote is for Option 2 for now, moving to 2b if we need more features.

chris-rock · 2016-09-15T14:33:01Z

I'd like options 2 with the possibility to use 2b in combination. Option 4 may become relevant later if we have more that resources to cover in a profile

stevendanna · 2016-09-16T09:06:10Z

I've been looking at 3 a little bit more this morning. I'm really skeptical that we'd be able to implement that without forcing the user to explicitly .call the returned resource. Maybe I'm just missing the obvious implementation though.

arlimus · 2016-09-16T09:55:01Z

Thank you for listing these options!

Option 1: Scrap due to naming + population of the namespace with profile names.
Option 2/3 are both good. 4 can be shortened to profile(xyz).resource(abc).

Let's think about 2 in how it's being used:

describe resource('profile_d', 'gordon_config', 'this_is_my_arg') do
  ...
end

This feels very long and clunky. But It'll do for now.

Option 3 is the cleanest solution in my opinion and establishes a good pattern, by separating the point of resource declaration and resource calling. Imho it's a method to extend the context with a pre-defined name:

require_resource(profile: 'profile_name', resource: 'resource_name')
require_resource(profile: 'profile_name', resource: 'other', as: 'renamed')

describe resource_name do
   ...
end 

describe renamed do
  ...
end

If you can implement it ^^, I'd 👍 this path.

stevendanna · 2016-09-16T10:05:00Z

4 can be shortened to profile(xyz).resource(abc).

Hrm, is abc here the name of the resource or the arguments to the resource. If the former, where would the arguments go?

The following proposal is pretty different than Option 3, let's call it Option 5:

require_resource(profile: 'profile_name', resource: 'resource_name')
require_resource(profile: 'profile_name', resource: 'other', as: 'renamed')

describe resource_name do
   ...
end 

describe renamed do
  ...
end

This is possible to implement (the problem with 3 is that I think we will run into issues with how the ruby parser works). It raises the question of whether, given a require_resource DSL method we would want to do any automatic loading of resources. That horse probably already left the stable because of existing profile code though.

I think I like this the best.

chris-rock · 2016-09-16T12:13:42Z

Since we return the resource, argument handling becomes as natural

require_resource(profile: 'profile_name', resource: 'resource_name')
describe resource_name(args) do
   ...
end

chris-rock · 2016-09-16T12:14:23Z

We need to verify that once I do require_resource, it works in our DSL and in our RSPec context.

chris-rock · 2016-09-16T12:41:08Z

@stevendanna great question if we should load resources by default or not. We need to find a smart way to do that. Right now, we allow single loading for resources, but what you want is more like a package loading eg.

require_resource(profile: 'profile_name', resource: '*')

Maybe that would be written as

require_resource profile: 'profile_name'
# or 
require_resource profile: 'profile_name', resource: '*'

But we could extend that later. For now we can focus on the special case where we have to resources named the same

stevendanna · 2016-09-16T12:52:45Z

We need to verify that once I do require_resource, it works in our DSL and in our RSPec context.

I've added a test for this case.

chris-rock · 2016-09-19T09:33:48Z

lib/inspec/runner.rb

@@ -182,58 +182,10 @@ def register_rules(ctx)

    private

-    def block_source_info(block)


Not sure why we move this to the RSpecRunner. the runner is the right place. By moving this to the RSpec runner, this method is not available to the mock runner anymore.

chris-rock · 2016-09-19T09:34:29Z

lib/inspec/control_eval_context.rb

@@ -92,6 +92,14 @@ def to_s
          res
        end

+        define_method :add_resource do |name, new_res|


Do you think we should make those methods private?

We can, but I'm not sure that will actually do what we want. Since currently control code gets executed via instance_exec on this class, It'll still have access to private methods.

Alright, lets keep them as is

chris-rock · 2016-09-19T09:36:04Z

lib/inspec/profile_context.rb

@@ -67,18 +68,31 @@ def profile_supports_os?

    def all_rules


we should rename this to all_controls and provide an alias for backwards-compatibility

chris-rock · 2016-09-19T09:38:12Z

lib/inspec/resource.rb

+          inner_context.resource_registry[resource_name]
+        end
+
+        define_method :resource do |profile_name, resource_name, *args|


Do we need that methods still?

That depends. Do we want a means of accessing specific resources without mutating the current namespace or not? If no, then we can certainly remove this.

chris-rock · 2016-09-19T16:27:05Z

test/unit/mock/profiles/dependencies/resource-namespace/controls/example.rb

+  end
+
+  describe gordy_config do
+    its('version') { should eq(gordy_config.version) }


All resources from deps are added into the control_eval_context used by the current profile. However, if there is a name conflict, the last loaded resource wins. The new `require_resource` dsl method allows the user to do the following: require_resource(profile: 'profile_name', resource: 'other', as: 'renamed') describe renamed do ... end Signed-off-by: Steven Danna <steve@chef.io>

chef-supermarket added the Signed CLA label Sep 15, 2016

stevendanna added the in progress label Sep 15, 2016

stevendanna force-pushed the ssd/resource-namespace branch from 568b818 to a82dc95 Compare September 15, 2016 08:32

chef-supermarket added the Signed CLA label Sep 15, 2016

stevendanna commented Sep 15, 2016

View reviewed changes

stevendanna force-pushed the ssd/resource-namespace branch from a82dc95 to 3130afc Compare September 15, 2016 08:36

chef-supermarket added the Signed CLA label Sep 15, 2016

stevendanna force-pushed the ssd/resource-namespace branch from 3130afc to 45cf5e2 Compare September 15, 2016 11:38

chef-supermarket added the Signed CLA label Sep 15, 2016

chris-rock reviewed Sep 15, 2016

View reviewed changes

stevendanna changed the title ~~WIP: Allow users to reference resources from dependencies~~ Allow users to reference resources from dependencies Sep 15, 2016

chef-supermarket added the Signed CLA label Sep 16, 2016

stevendanna force-pushed the ssd/resource-namespace branch from 5b66790 to 850a430 Compare September 19, 2016 08:54