On Fri, 03 Sep 2010 11:23:46 -0700, Paul Berry wrote: > > 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. > > Signed-off-by: Paul Berry <[email protected]> > --- > lib/puppet/dsl.rb | 4 - > lib/puppet/dsl/resource_type_api.rb | 22 ++-- > lib/puppet/parser/parser_support.rb | 4 +- > spec/integration/parser/ruby_manifest_spec.rb | 127 > +++++++++++++++++++++++++ > spec/unit/dsl/resource_type_api_spec.rb | 44 ++++++--- > spec/unit/parser/parser_spec.rb | 9 -- > 6 files changed, 173 insertions(+), 37 deletions(-) > create mode 100644 spec/integration/parser/ruby_manifest_spec.rb > > diff --git a/lib/puppet/dsl.rb b/lib/puppet/dsl.rb > index abdb78f..97a3104 100644 > --- a/lib/puppet/dsl.rb > +++ b/lib/puppet/dsl.rb > @@ -5,7 +5,3 @@ end > > require 'puppet/dsl/resource_type_api' > require 'puppet/dsl/resource_api' > - > -class Object > - include Puppet::DSL::ResourceTypeAPI > -end > diff --git a/lib/puppet/dsl/resource_type_api.rb > b/lib/puppet/dsl/resource_type_api.rb > index 487aab9..90dd5b2 100644 > --- a/lib/puppet/dsl/resource_type_api.rb > +++ b/lib/puppet/dsl/resource_type_api.rb > @@ -1,33 +1,33 @@ > require 'puppet/resource/type' > > -module Puppet::DSL::ResourceTypeAPI > +class Puppet::DSL::ResourceTypeAPI > def define(name, *args, &block) > - result = mk_resource_type(:definition, name, Hash.new, block) > - result.set_arguments(munge_type_arguments(args)) > - result > + result = __mk_resource_type__(:definition, name, Hash.new, block) > + result.set_arguments(__munge_type_arguments__(args)) > + nil > end > > def hostclass(name, options = {}, &block) > - mk_resource_type(:hostclass, name, options, block) > + __mk_resource_type__(:hostclass, name, options, block) > + nil > end > > def node(name, options = {}, &block) > - mk_resource_type(:node, name, options, block) > + __mk_resource_type__(:node, name, options, block) > + nil > end > > - private > - > - def mk_resource_type(type, name, options, code) > + def __mk_resource_type__(type, name, options, code) > klass = Puppet::Resource::Type.new(type, name, options)
We already discussed this in person, but: There should be a comment here
stating why "private" is insufficient protection on these methods.
>
> klass.ruby_code = code if code
>
> - Puppet::Node::Environment.new.known_resource_types.add klass
> + Thread.current[:known_resource_types].add klass
>
> klass
> end
>
> - def munge_type_arguments(args)
> + def __munge_type_arguments__(args)
> args.inject([]) do |result, item|
> if item.is_a?(Hash)
> item.each { |p, v| result << [p, v] }
> diff --git a/lib/puppet/parser/parser_support.rb
> b/lib/puppet/parser/parser_support.rb
> index c0fd371..25bf25f 100644
> --- a/lib/puppet/parser/parser_support.rb
> +++ b/lib/puppet/parser/parser_support.rb
> @@ -215,7 +215,9 @@ class Puppet::Parser::Parser
> end
>
> def parse_ruby_file
> - require self.file
> + # Execute the contents of the file inside its own "main" object so
> + # that it can call methods in the resource type API.
> + Puppet::DSL::ResourceTypeAPI.new.instance_eval(File.read(self.file))
> end
>
> def string=(string)
> diff --git a/spec/integration/parser/ruby_manifest_spec.rb
> b/spec/integration/parser/ruby_manifest_spec.rb
> new file mode 100644
> index 0000000..9ff7805
> --- /dev/null
> +++ b/spec/integration/parser/ruby_manifest_spec.rb
> @@ -0,0 +1,127 @@
> +#!/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 "foo", :arg do
> notify("foo...@name}_#{@arg}") end')
> + catalog = compile("import 'foo'\nfoo { instance: arg => 'xyz' }")
> + catalog.resource("Notify[foo_instance_xyz]").should_not be_nil
> + catalog.resource("Foo[instance]").should_not be_nil
> + end
> +
> + it "should allow node declarations" do
> + write_file('foo.rb', ["node 'mynode' do notify('mynode') end",
> + "node 'theirnode' do notify('theirnode')
> end"].join("\n"))
> + catalog = compile("import 'foo'")
> + catalog.resource("Notify[mynode]").should_not be_nil
> + catalog.resource("Notify[theirnode]").should be_nil
> + end
The "should ..." text should probably be expanded to state that 'mynode'
is the current node, and that is why it is not nil. Perhaps adding this
to the describe, would be better ("Pure Ruby manifests with node
'mynode'")?
> +
> + it "should allow access to the environment" do
> + write_file('foo.rb', ["hostclass 'foo' do",
> + " if environment.is_a? Puppet::Node::Environment",
> + " notify('success')",
> + " end",
> + "end"].join("\n"))
> + compile("import 'foo'\ninclude
> foo").resource("Notify[success]").should_not be_nil
> + end
> +
> + it "should allow creation of built-in types" do
> + write_file('foo.rb', "hostclass 'foo' do file 'test_file', :owner =>
> 'root', :mode => '644' end")
> + catalog = compile("import 'foo'\ninclude foo")
> + 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
From the description it sounded like this test was making sure that you
could create new types, not use existing ones. Yay, English.
> +
> + it "should allow calling user-defined functions" do
> + write_file('foo.rb', "hostclass 'foo' do user_func :arg => 'xyz' end")
> + catalog = compile(['define user_func($arg) { notify {"n_$arg": } }',
> + 'user_func { "one": arg => "two" }'].join("\n"))
> + catalog.resource("Notify[n_two]").should_not be_nil
> + catalog.resource("User_func[one]").should_not be_nil
> + end
Removing the 'write_file' allows this test to still pass. Is it
really an un-needed vestige, or is something else going on?
> +
> + 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 'foo' do notify('success') end")
> + Puppet[:code] = "import 'foo'\ninclude foo"
This idiom appeared earlier of having the hostclass named the same as
the file. While it makes sense, it's also a little confusing in the
tests. Perhaps they should not have the same name to emphasize exactly
what is going on?
> +
> + # 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
Perhaps check that the new file wasn't somehow added to the existing
one? IOW: "hostclass 'foo' do notify('this should not be here') end"
catalog.resource("Notify[this should not be here]").should be_nil
Similar to how you did the test below.
> + 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 'foo' do notify('version1') end")
> + catalog = compile("import 'foo'\ninclude foo")
> + catalog.resource("Notify[version1]").should_not be_nil
> + sleep 1 # so that timestamp will change forcing file reload
> + write_file('foo.rb', "hostclass 'foo' do notify('version2') end")
> + catalog = compile("import 'foo'\ninclude foo")
> + 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 5abe79e..ba396ac 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=)
> + test_api_call { send(method, "myname") { 'method_result' } }
> + end
This really only tests that the method was called, not that it actually
"set[s] any provided block as the type's ruby code", maybe add
".with(...)" to the .expects to ensure it gets the correct value? Kinda
like how you did the test below.
>
> - 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 0657ab3..f73e07a 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
> --
> 1.7.2
>
--
Jacob Helwig
signature.asc
Description: Digital signature
