[#4716] ResourceTypeAPI exposes implementation details that are likely to change

Made the following modifications to ResourceTypeAPI:

(1) returned nil from “define”, “hostclass”, and “node”.

(2) renamed “mk_resource_type” and “munge_type_arguments” to
“__mk_resource_type__” and “__munge_type_arguments__” to discourage
customers from calling them.

(3) Made ResourceTypeAPI a class rather than a module, and changed the
parser to evaluate the contents of pure ruby manifests using a
instances of this class.

(4) Changed ResourceTypeAPI to insert newly instantiated types into
Thread.current[:known_resource_types] rather than the default
environment's known_resource_types.

This effectively backports the fix for issue #4657 to 2.6.x.

Also backported the new spec tests from #4657.

3 files changed, 160 insertions, 21 deletions

diff --git a/spec/integration/parser/ruby_manifest_spec.rb b/spec/integration/parser/ruby_manifest_spec.rb
new file mode 100644
index 000000000..af110d3b3
--- /dev/null
+++ b/spec/integration/parser/ruby_manifest_spec.rb
@@ -0,0 +1,128 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+
+require 'tempfile'
+require 'puppet_spec/files'
+
+describe "Pure ruby manifests" do
+  include PuppetSpec::Files
+
+  before do
+    @node = Puppet::Node.new "testnode"
+
+    @scope_resource = stub 'scope_resource', :builtin? => true, :finish => nil, :ref => 'Class[main]'
+    @scope = stub 'scope', :resource => @scope_resource, :source => mock("source")
+    @test_dir = tmpdir('ruby_manifest_test')
+  end
+
+  after do
+    Puppet.settings.clear
+  end
+
+  def write_file(name, contents)
+    path = File.join(@test_dir, name)
+    File.open(path, "w") { |f| f.write(contents) }
+    path
+  end
+
+  def compile(contents)
+    Puppet[:code] = contents
+    Dir.chdir(@test_dir) do
+      Puppet::Parser::Compiler.compile(Puppet::Node.new("mynode"))
+    end
+  end
+
+  it "should allow classes" do
+    write_file('foo.rb', ["hostclass 'one' do notify('one_notify') end",
+                          "hostclass 'two' do notify('two_notify') end"].join("\n"))
+    catalog = compile("import 'foo'\ninclude one")
+    catalog.resource("Notify[one_notify]").should_not be_nil
+    catalog.resource("Notify[two_notify]").should be_nil
+  end
+
+  it "should allow defines" do
+    write_file('foo.rb', 'define "bar", :arg do notify("bar_#{@name}_#{@arg}") end')
+    catalog = compile("import 'foo'\nbar { instance: arg => 'xyz' }")
+    catalog.resource("Notify[bar_instance_xyz]").should_not be_nil
+    catalog.resource("Bar[instance]").should_not be_nil
+  end
+
+  it "should allow node declarations" do
+    write_file('foo.rb', "node 'mynode' do notify('mynode') end")
+    catalog = compile("import 'foo'")
+    node_declaration = catalog.resource("Notify[mynode]")
+    node_declaration.should_not be_nil
+    node_declaration.title.should == 'mynode'
+  end
+
+  it "should allow access to the environment" do
+    write_file('foo.rb', ["hostclass 'bar' do",
+                          "  if environment.is_a? Puppet::Node::Environment",
+                          "    notify('success')",
+                          "  end",
+                          "end"].join("\n"))
+    compile("import 'foo'\ninclude bar").resource("Notify[success]").should_not be_nil
+  end
+
+  it "should allow creation of resources of built-in types" do
+    write_file('foo.rb', "hostclass 'bar' do file 'test_file', :owner => 'root', :mode => '644' end")
+    catalog = compile("import 'foo'\ninclude bar")
+    file = catalog.resource("File[test_file]")
+    file.should be_a Puppet::Resource
+    file.type.should == 'File'
+    file.title.should == 'test_file'
+    file.exported.should_not be
+    file.virtual.should_not be
+    file[:owner].should == 'root'
+    file[:mode].should == '644'
+    file[:stage].should be_nil # TODO: is this correct behavior?
+  end
+
+  it "should allow calling user-defined functions" do
+    write_file('foo.rb', "hostclass 'bar' do user_func 'name', :arg => 'xyz' end")
+    catalog = compile(['define user_func($arg) { notify {"n_$arg": } }',
+                       'import "foo"',
+                       'include bar'].join("\n"))
+    catalog.resource("Notify[n_xyz]").should_not be_nil
+    catalog.resource("User_func[name]").should_not be_nil
+  end
+
+  it "should be properly cached for multiple compiles" do
+    # Note: we can't test this by calling compile() twice, because
+    # that sets Puppet[:code], which clears out all cached
+    # environments.
+    Puppet[:filetimeout] = 1000
+    write_file('foo.rb', "hostclass 'bar' do notify('success') end")
+    Puppet[:code] = "import 'foo'\ninclude bar"
+
+    # Compile the catalog and check it
+    catalog = Dir.chdir(@test_dir) do
+      Puppet::Parser::Compiler.compile(Puppet::Node.new("mynode"))
+    end
+    catalog.resource("Notify[success]").should_not be_nil
+
+    # Secretly change the file to make it invalid.  This change
+    # shouldn't be noticed because the we've set a high
+    # Puppet[:filetimeout].
+    write_file('foo.rb', "raise 'should not be executed'")
+
+    # Compile the catalog a second time and make sure it's still ok.
+    catalog = Dir.chdir(@test_dir) do
+      Puppet::Parser::Compiler.compile(Puppet::Node.new("mynode"))
+    end
+    catalog.resource("Notify[success]").should_not be_nil
+  end
+
+  it "should be properly reloaded when stale" do
+    Puppet[:filetimeout] = -1 # force stale check to happen all the time
+    write_file('foo.rb', "hostclass 'bar' do notify('version1') end")
+    catalog = compile("import 'foo'\ninclude bar")
+    catalog.resource("Notify[version1]").should_not be_nil
+    sleep 1 # so that timestamp will change forcing file reload
+    write_file('foo.rb', "hostclass 'bar' do notify('version2') end")
+    catalog = compile("import 'foo'\ninclude bar")
+    catalog.resource("Notify[version1]").should be_nil
+    catalog.resource("Notify[version2]").should_not be_nil
+  end
+end
diff --git a/spec/unit/dsl/resource_type_api_spec.rb b/spec/unit/dsl/resource_type_api_spec.rb
index 5abe79ea7..4f4eb7e01 100755
--- a/spec/unit/dsl/resource_type_api_spec.rb
+++ b/spec/unit/dsl/resource_type_api_spec.rb
@@ -4,13 +4,14 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 
 require 'puppet/dsl/resource_type_api'
 
-class DSLAPITester
-  include Puppet::DSL::ResourceTypeAPI
-end
-
 describe Puppet::DSL::ResourceTypeAPI do
-  before do
-    @api = DSLAPITester.new
+  # Run the given block in the context of a new ResourceTypeAPI
+  # object.
+  def test_api_call(&block)
+    Thread.current[:known_resource_types] = Puppet::Resource::TypeCollection.new(:env)
+    Puppet::DSL::ResourceTypeAPI.new.instance_eval(&block)
+  ensure
+    Thread.current[:known_resource_types] = nil
   end
 
   [:definition, :node, :hostclass].each do |type|
@@ -18,29 +19,48 @@ describe Puppet::DSL::ResourceTypeAPI do
     it "should be able to create a #{type}" do
       newtype = Puppet::Resource::Type.new(:hostclass, "foo")
       Puppet::Resource::Type.expects(:new).with { |t, n, args| t == type }.returns newtype
-      @api.send(method, "myname")
+      test_api_call { send(method, "myname") }
     end
 
     it "should use the provided name when creating a #{type}" do
       type = Puppet::Resource::Type.new(:hostclass, "foo")
       Puppet::Resource::Type.expects(:new).with { |t, n, args| n == "myname" }.returns type
-      @api.send(method, "myname")
+      test_api_call { send(method, "myname") }
     end
 
     unless type == :definition
       it "should pass in any provided options" do
         type = Puppet::Resource::Type.new(:hostclass, "foo")
         Puppet::Resource::Type.expects(:new).with { |t, n, args| args == {:myarg => :myvalue} }.returns type
-        @api.send(method, "myname", :myarg => :myvalue)
+        test_api_call { send(method, "myname", :myarg => :myvalue) }
       end
     end
 
-    it "should set any provided block as the type's ruby code"
+    it "should set any provided block as the type's ruby code" do
+      Puppet::Resource::Type.any_instance.expects(:ruby_code=).with { |blk| blk.call == 'foo' }
+      test_api_call { send(method, "myname") { 'foo' } }
+    end
 
-    it "should add the type to the current environment's known resource types"
+    it "should add the type to the current environment's known resource types" do
+      begin
+        newtype = Puppet::Resource::Type.new(:hostclass, "foo")
+        Puppet::Resource::Type.expects(:new).returns newtype
+        known_resource_types = Puppet::Resource::TypeCollection.new(:env)
+        Thread.current[:known_resource_types] = known_resource_types
+        known_resource_types.expects(:add).with(newtype)
+        Puppet::DSL::ResourceTypeAPI.new.instance_eval { hostclass "myname" }
+      ensure
+        Thread.current[:known_resource_types] = nil
+      end
+    end
   end
 
   describe "when creating a definition" do
-    it "should use the provided options to define valid arguments for the resource type"
+    it "should use the provided options to define valid arguments for the resource type" do
+      newtype = Puppet::Resource::Type.new(:definition, "foo")
+      Puppet::Resource::Type.expects(:new).returns newtype
+      test_api_call { define("myname", :arg1, :arg2) }
+      newtype.instance_eval { @arguments }.should == { 'arg1' => nil, 'arg2' => nil }
+    end
   end
 end
diff --git a/spec/unit/parser/parser_spec.rb b/spec/unit/parser/parser_spec.rb
index 0657ab37a..f73e07a5c 100755
--- a/spec/unit/parser/parser_spec.rb
+++ b/spec/unit/parser/parser_spec.rb
@@ -52,15 +52,6 @@ describe Puppet::Parser do
       @parser.file = "/my/file.rb"
       @parser.parse
     end
-
-    describe "in ruby" do
-      it "should use the ruby interpreter to load the file" do
-        @parser.file = "/my/file.rb"
-        @parser.expects(:require).with "/my/file.rb"
-
-        @parser.parse_ruby_file
-      end
-    end
   end
 
   describe "when parsing append operator" do