Adding #539. Definitions can now have titles, and both $title and $name are guaranteed to be set within any definition.

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@2301 980ebf18-57e1-0310-9a29-db15c13687c0

5 files changed, 108 insertions, 12 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 4b7ad9ddd..f01a18246 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,4 +1,7 @@
 0.22.2 (grover)
+    Definitions now support both 'name' and 'title', just like any other
+    resource type. (#539)
+
     Added a generate() command, which sets values to the result of an external
     command. (#541)
 
diff --git a/lib/puppet/parser/ast/component.rb b/lib/puppet/parser/ast/component.rb
index 8f82d530a..9e3179edb 100644
--- a/lib/puppet/parser/ast/component.rb
+++ b/lib/puppet/parser/ast/component.rb
@@ -28,13 +28,15 @@ class Puppet::Parser::AST
         def evaluate(hash)
             origscope = hash[:scope]
             objtype = hash[:type]
-            name = hash[:name]
+            title = hash[:title]
             args = symbolize_options(hash[:arguments] || {})
 
+            name = args[:name] || title
+
             exported = hash[:exported]
 
             pscope = origscope
-            scope = subscope(pscope, name)
+            scope = subscope(pscope, title)
 
             if exported or origscope.exported?
                 scope.exported = true
@@ -46,8 +48,10 @@ class Puppet::Parser::AST
                 scope.tag(@type)
             end
 
-            unless name.nil? or name =~ /[^\w]/ or name == ""
-                scope.tag(name)
+            [name, title].each do |str|
+                unless str.nil? or str =~ /[^\w]/ or str == ""
+                    scope.tag(str)
+                end
             end
 
             # define all of the arguments in our local scope
@@ -66,7 +70,7 @@ class Puppet::Parser::AST
                             #    [default.inspect, arg.inspect, @name.inspect]
                         else
                             parsefail "Must pass %s to %s of type %s" %
-                                    [arg,name,@type]
+                                    [arg,title,@type]
                         end
                     end
                 }
@@ -84,7 +88,11 @@ class Puppet::Parser::AST
                 end
             }
 
-            unless args.include? "name"
+            unless args.include? :title
+                scope.setvar("title",title)
+            end
+
+            unless args.include? :name
                 scope.setvar("name",name)
             end
 
@@ -192,6 +200,8 @@ class Puppet::Parser::AST
             if @arguments.include?(param)
                 # It's a valid arg for us
                 return true
+            elsif param == "name"
+                return true
 #            elsif defined? @parentclass and @parentclass
 #                # Else, check any existing parent
 #                if parent = @scope.lookuptype(@parentclass) and parent != []
diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb
index 31d43bd18..bdace28cd 100644
--- a/lib/puppet/parser/resource.rb
+++ b/lib/puppet/parser/resource.rb
@@ -91,7 +91,7 @@ class Puppet::Parser::Resource
             scope.deleteresource(self)
             return klass.evaluate(:scope => scope,
                                   :type => self.type,
-                                  :name => self.title,
+                                  :title => self.title,
                                   :arguments => self.to_hash,
                                   :scope => self.scope,
                                   :exported => self.exported
@@ -182,16 +182,17 @@ class Puppet::Parser::Resource
     # Verify that all passed parameters are valid.  This throws an error if there's
     # a problem, so we don't have to worry about the return value.
     def paramcheck(param)
+        param = param.to_s
         # Now make sure it's a valid argument to our class.  These checks
         # are organized in order of commonhood -- most types, it's a valid argument
         # and paramcheck is enabled.
         if @ref.typeclass.validattr?(param)
             true
-        elsif (param == "name" or param == "title") # always allow these
+        elsif %w{name title}.include?(param) # always allow these
             true
         elsif paramcheck?
             self.fail Puppet::ParseError, "Invalid parameter '%s' for type '%s'" %
-                    [param, @ref.type]
+                    [param.inspect, @ref.type]
         end
     end
 
diff --git a/test/language/ast/component.rb b/test/language/ast/component.rb
index 0492d692d..1500aa397 100755
--- a/test/language/ast/component.rb
+++ b/test/language/ast/component.rb
@@ -6,6 +6,7 @@
 $:.unshift("../../lib") if __FILE__ =~ /\.rb$/
 
 require 'puppettest'
+require 'mocha'
 require 'puppettest/parsertesting'
 require 'puppettest/resourcetesting'
 
@@ -49,7 +50,7 @@ class TestASTComponent < Test::Unit::TestCase
 
         assert_nothing_raised do
             klass.evaluate(:scope => scope,
-                :name => "first",
+                :title => "first",
                 :arguments => {"mode" => "755"}
             )
         end
@@ -65,7 +66,7 @@ class TestASTComponent < Test::Unit::TestCase
         # Make sure we can't evaluate it with the same args
         assert_raise(Puppet::ParseError) do
             klass.evaluate(:scope => scope,
-                :name => "first",
+                :title => "first",
                 :arguments => {"mode" => "755"}
             )
         end
@@ -73,7 +74,7 @@ class TestASTComponent < Test::Unit::TestCase
         # Now create another with different args
         assert_nothing_raised do
             klass.evaluate(:scope => scope,
-                :name => "second",
+                :title => "second",
                 :arguments => {"mode" => "755", "owner" => "daemon"}
             )
         end
@@ -86,5 +87,47 @@ class TestASTComponent < Test::Unit::TestCase
         assert_equal("daemon", secondobj[:owner])
         assert_equal("755", secondobj[:mode])
     end
+
+    # #539 - definitions should support both names and titles
+    def test_names_and_titles
+        interp, scope, source = mkclassframing
+
+        [
+        {:name => "one", :title => "two"},
+        {:title => "mytitle"},
+        ].each_with_index do |hash, i|
+
+            # Create a definition that uses both name and title
+            klass = interp.newdefine "yayness%s" % i
+
+            subscope = klass.subscope(scope, "yayness%s" % i)
+
+            klass.expects(:subscope).returns(subscope)
+
+            args = {:title => hash[:title]}
+            if hash[:name]
+                args[:arguments] = {:name => hash[:name]}
+            end
+            args[:scope] = scope
+            assert_nothing_raised("Could not evaluate definition with %s" % hash.inspect) do
+                klass.evaluate(args)
+            end
+
+            name = hash[:name] || hash[:title]
+            title = hash[:title]
+            args[:name] ||= name
+
+            assert_equal(name, subscope.lookupvar("name"),
+                "Name did not get set correctly")
+            assert_equal(title, subscope.lookupvar("title"),
+                "title did not get set correctly")
+
+            [:name, :title].each do |param|
+                val = args[param]
+                assert(subscope.tags.include?(val),
+                    "Scope was not tagged with %s" % val)
+            end
+        end
+    end
 end
 # $Id$
diff --git a/test/language/resource.rb b/test/language/resource.rb
index 92a4aa840..8614d4ba8 100755
--- a/test/language/resource.rb
+++ b/test/language/resource.rb
@@ -487,6 +487,45 @@ class TestResource < Test::Unit::TestCase
                 [param, value])
         end
     end
+
+    # The second part of #539 - make sure resources pass the arguments
+    # correctly.
+    def test_title_with_definitions
+        define = @interp.newdefine "yayness",
+            :code => resourcedef("file", "/tmp",
+                "owner" => varref("name"), "mode" => varref("title"))
+
+        klass = @interp.findclass("", "")
+        should = {:name => :owner, :title => :mode}
+        [
+        {:name => "one", :title => "two"},
+        {:title => "three"},
+        ].each do |hash|
+            scope = mkscope :interp => @interp
+            args = {:type => "yayness", :title => hash[:title],
+                :source => klass, :scope => scope}
+            if hash[:name]
+                args[:params] = {:name => hash[:name]}
+            else
+                args[:params] = {} # override the defaults
+            end
+
+            res = nil
+            assert_nothing_raised("Could not create res with %s" % hash.inspect) do
+                res = mkresource(args)
+            end
+            assert_nothing_raised("Could not eval res with %s" % hash.inspect) do
+                res.evaluate
+            end
+
+            made = scope.findresource("File[/tmp]")
+            assert(made, "Did not create resource with %s" % hash.inspect)
+            should.each do |orig, param|
+                assert_equal(hash[orig] || hash[:title], made[param],
+                    "%s was not set correctly with %s" % [param, hash.inspect])
+            end
+        end
+    end
 end
 
 # $Id$