Merge branch '0.24.x'

Conflicts:
	test/ral/manager/type.rb

13 files changed, 186 insertions, 106 deletions

diff --git a/CHANGELOG b/CHANGELOG
index df1f8ecf7..d81a0424f 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -14,6 +14,9 @@
     Fixed #1840 - Bug fixes and improvements for Emacs puppet-mode.el
 
 0.24.8
+    Fixed #1956 - Cleaned up variable names to be more sane, clarified error messages 
+    and fixed incorrect use of 'value' variable rather than 'member'.
+    
     Fixed #1831 - Added sprintf function
 
     Fixed #1830 - Added regsubst function
diff --git a/lib/puppet/provider/nameservice/directoryservice.rb b/lib/puppet/provider/nameservice/directoryservice.rb
index 4e21d4169..42c52f0a7 100644
--- a/lib/puppet/provider/nameservice/directoryservice.rb
+++ b/lib/puppet/provider/nameservice/directoryservice.rb
@@ -15,6 +15,7 @@
 require 'puppet'
 require 'puppet/provider/nameservice'
 require 'facter/util/plist'
+require 'cgi'
 
 
 class Puppet::Provider::NameService
@@ -26,6 +27,7 @@ class DirectoryService < Puppet::Provider::NameService
         #  e.g. Puppet::Type.type(:user).provide :directoryservice, :ds_path => "Users"
         #  This is referenced in the get_ds_path class method
         attr_writer :ds_path
+        attr_writer :macosx_version_major
     end
 
     
@@ -40,6 +42,7 @@ class DirectoryService < Puppet::Provider::NameService
     
     commands :dscl => "/usr/bin/dscl"
     commands :dseditgroup => "/usr/sbin/dseditgroup"
+    commands :sw_vers => "/usr/bin/sw_vers"
     confine :operatingsystem => :darwin
     defaultfor :operatingsystem => :darwin
 
@@ -100,12 +103,31 @@ class DirectoryService < Puppet::Provider::NameService
         #   @ds_path is an attribute of the class itself.  
         if defined? @ds_path
             return @ds_path
-        else
-            # JJM: "Users" or "Groups" etc ...  (Based on the Puppet::Type)
-            #       Remember this is a class method, so self.class is Class
-            #       Also, @resource_type seems to be the reference to the
-            #       Puppet::Type this class object is providing for.
-            return @resource_type.name.to_s.capitalize + "s"
+        end
+        # JJM: "Users" or "Groups" etc ...  (Based on the Puppet::Type)
+        #       Remember this is a class method, so self.class is Class
+        #       Also, @resource_type seems to be the reference to the
+        #       Puppet::Type this class object is providing for.
+        return @resource_type.name.to_s.capitalize + "s"
+    end
+    
+    def self.get_macosx_version_major
+        if defined? @macosx_version_major
+            return @macosx_version_major
+        end
+        begin
+            product_version = Facter.value(:macosx_productversion)
+            if product_version.nil?
+                raise Puppet::Error, "Could not determine OS X version: %s" % detail
+            end
+            product_version_major = product_version.scan(/(\d+)\.(\d+)./).join(".")
+            if %w{10.0 10.1 10.2 10.3}.include?(product_version_major)
+                raise Puppet::Error, "%s is not supported by the directoryservice provider" % product_version_major
+            end
+            @macosx_version_major = product_version_major
+            return @macosx_version_major
+        rescue Puppet::ExecutionFailure => detail
+            raise Puppet::Error, "Could not determine OS X version: %s" % detail
         end
     end
 
@@ -119,34 +141,56 @@ class DirectoryService < Puppet::Provider::NameService
         return dscl_output.split("\n")
     end
     
-    def self.single_report(resource_name, *type_properties)
-        # JJM 2007-07-24:
-        #     Given a the name of an object and a list of properties of that
-        #     object, return all property values in a hash.
-        #     
-        #     This class method returns nil if the object doesn't exist
-        #     Otherwise, it returns a hash of the object properties.
-        
-        all_present_str_array = list_all_present()
-        
-        # NBK: shortcut the process if the resource is missing
-        return nil unless all_present_str_array.include? resource_name
+    def self.parse_dscl_url_data(dscl_output)
+        # we need to construct a Hash from the dscl -url output to match
+        # that returned by the dscl -plist output for 10.5+ clients.
+        #
+        # Nasty assumptions:
+        #   a) no values *end* in a colon ':', only keys
+        #   b) if a line ends in a colon and the next line does start with
+        #      a space, then the second line is a value of the first.
+        #   c) (implied by (b)) keys don't start with spaces.
         
-        dscl_vector = get_exec_preamble("-read", resource_name)
-        begin
-            dscl_output = execute(dscl_vector)
-        rescue Puppet::ExecutionFailure => detail
-            raise Puppet::Error, "Could not get report.  command execution failed."
+        dscl_plist = {}
+        dscl_output.split("\n").inject([]) do |array, line|
+          if line =~ /^\s+/   # it's a value
+            array[-1] << line # add the value to the previous key
+          else
+            array << line
+          end
+          array
+        end.compact
+
+        dscl_output.each do |line|
+            # This should be a 'normal' entry. key and value on one line.
+            # We split on ': ' to deal with keys/values with a colon in them.
+            split_array = line.split(/:\s+/)
+            key = split_array.first
+            value = CGI::unescape(split_array.last.strip.chomp)
+            # We need to treat GroupMembership separately as it is currently
+            # the only attribute we care about multiple values for, and
+            # the values can never contain spaces (shortnames)
+            # We also make every value an array to be consistent with the
+            # output of dscl -plist under 10.5
+            if key == "GroupMembership"
+                dscl_plist[key] = value.split(/\s/)
+            else
+                dscl_plist[key] = [value]
+            end
         end
-        
-        # JJM: We need a new hash to return back to our caller.
-        attribute_hash = Hash.new
-        
-        dscl_plist = Plist.parse_xml(dscl_output)
-        dscl_plist.keys().each do |key|
+        return dscl_plist
+    end
+    
+    def self.parse_dscl_plist_data(dscl_output)
+        return Plist.parse_xml(dscl_output)
+    end
+    
+    def self.generate_attribute_hash(input_hash, *type_properties)
+        attribute_hash = {}
+        input_hash.keys().each do |key|
             ds_attribute = key.sub("dsAttrTypeStandard:", "")
             next unless (@@ds_to_ns_attribute_map.keys.include?(ds_attribute) and type_properties.include? @@ds_to_ns_attribute_map[ds_attribute])
-            ds_value = dscl_plist[key]
+            ds_value = input_hash[key]
             case @@ds_to_ns_attribute_map[ds_attribute]
                 when :members: 
                     ds_value = ds_value # only members uses arrays so far
@@ -172,7 +216,41 @@ class DirectoryService < Puppet::Provider::NameService
         if @resource_type.validproperties.include?(:password)
             attribute_hash[:password] = self.get_password(attribute_hash[:guid])
         end
-        return attribute_hash    
+        return attribute_hash
+    end
+    
+    def self.single_report(resource_name, *type_properties)
+        # JJM 2007-07-24:
+        #     Given a the name of an object and a list of properties of that
+        #     object, return all property values in a hash.
+        #     
+        #     This class method returns nil if the object doesn't exist
+        #     Otherwise, it returns a hash of the object properties.
+        
+        all_present_str_array = list_all_present()
+        
+        # NBK: shortcut the process if the resource is missing
+        return nil unless all_present_str_array.include? resource_name
+        
+        dscl_vector = get_exec_preamble("-read", resource_name)
+        begin
+            dscl_output = execute(dscl_vector)
+        rescue Puppet::ExecutionFailure => detail
+            raise Puppet::Error, "Could not get report.  command execution failed."
+        end
+        
+        # Two code paths is ugly, but until we can drop 10.4 support we don't
+        # have a lot of choice. Ultimately this should all be done using Ruby
+        # to access the DirectoryService APIs directly, but that's simply not
+        # feasible for a while yet.
+        case self.get_macosx_version_major
+        when "10.4"
+            dscl_plist = self.parse_dscl_url_data(dscl_output)
+        when "10.5", "10.6"
+            dscl_plist = self.parse_dscl_plist_data(dscl_output)
+        end
+        
+        return self.generate_attribute_hash(dscl_plist, *type_properties)
     end
     
     def self.get_exec_preamble(ds_action, resource_name = nil)
@@ -183,8 +261,16 @@ class DirectoryService < Puppet::Provider::NameService
         #     This method spits out proper DSCL commands for us.
         #     We EXPECT name to be @resource[:name] when called from an instance object.
 
-        # There are two ways to specify paths in 10.5.  See man dscl.
-        command_vector = [ command(:dscl), "-plist", "." ]
+        # 10.4 doesn't support the -plist option for dscl, and 10.5 has a
+        # different format for the -url output with objects with spaces in
+        # their values. *sigh*. Use -url for 10.4 in the hope this can be
+        # deprecated one day, and use -plist for 10.5 and higher.
+        case self.get_macosx_version_major
+        when "10.4"
+            command_vector = [ command(:dscl), "-url", "." ]
+        when "10.5", "10.6"
+            command_vector = [ command(:dscl), "-plist", "." ]
+        end
         # JJM: The actual action to perform.  See "man dscl"
         #      Common actiosn: -create, -delete, -merge, -append, -passwd
         command_vector << ds_action
@@ -234,7 +320,6 @@ class DirectoryService < Puppet::Provider::NameService
     def self.get_password(guid)
         password_hash = nil
         password_hash_file = "#{@@password_hash_dir}/#{guid}"
-        # TODO: sort out error conditions?
         if File.exists?(password_hash_file)
             if not File.readable?(password_hash_file)
                 raise Puppet::Error("Could not read password hash file at #{password_hash_file} for #{@resource[:name]}")
@@ -322,9 +407,8 @@ class DirectoryService < Puppet::Provider::NameService
     # to create objects with dscl, rather than the single command nameservice.rb
     # expects to be returned by addcmd. Thus we don't bother defining addcmd.
     def create
-       if exists?
+        if exists?
             info "already exists"
-            # The object already exists
             return nil
         end
         
@@ -373,25 +457,25 @@ class DirectoryService < Puppet::Provider::NameService
     
     def remove_unwanted_members(current_members, new_members)
         current_members.each do |member|
-            if not value.include?(member)
+            if not new_members.include?(member)
                 cmd = [:dseditgroup, "-o", "edit", "-n", ".", "-d", member, @resource[:name]]
                 begin
                      execute(cmd)
                 rescue Puppet::ExecutionFailure => detail
-                     raise Puppet::Error, "Could not set %s on %s[%s]: %s" % [param, @resource.class.name, @resource.name, detail]
+                     raise Puppet::Error, "Could not remove %s from group: %s, %s" % [member, @resource.name, detail]
                 end
              end
          end
     end
     
     def add_members(current_members, new_members)
-        new_members.each do |user|
-           if current_members.nil? or not current_members.include?(user)
-               cmd = [:dseditgroup, "-o", "edit", "-n", ".", "-a", user, @resource[:name]]
+        new_members.each do |new_member|
+           if current_members.nil? or not current_members.include?(new_member)
+               cmd = [:dseditgroup, "-o", "edit", "-n", ".", "-a", new_member, @resource[:name]]
                begin
                     execute(cmd)
                rescue Puppet::ExecutionFailure => detail
-                    raise Puppet::Error, "Could not set %s on %s[%s]: %s" % [param, @resource.class.name, @resource.name, detail]
+                    raise Puppet::Error, "Could not add %s to group: %s, %s" % [new_member, @resource.name, detail]
                end
            end
         end
diff --git a/lib/puppet/provider/package/up2date.rb b/lib/puppet/provider/package/up2date.rb
index 5708905cc..d8a12652f 100644
--- a/lib/puppet/provider/package/up2date.rb
+++ b/lib/puppet/provider/package/up2date.rb
@@ -4,7 +4,7 @@ Puppet::Type.type(:package).provide :up2date, :parent => :rpm, :source => :rpm d
 
     commands :up2date => "/usr/sbin/up2date-nox"
 
-    defaultfor :operatingsystem => [:redhat, :oel, :ovm] 
+    defaultfor :operatingsystem => [:redhat, :oel, :ovm],
                :lsbdistrelease => ["2.1", "3", "4"]
 
     confine    :operatingsystem => [:redhat, :oel, :ovm]
diff --git a/lib/puppet/provider/parsedfile.rb b/lib/puppet/provider/parsedfile.rb
index 45eae57ff..40e172785 100755
--- a/lib/puppet/provider/parsedfile.rb
+++ b/lib/puppet/provider/parsedfile.rb
@@ -81,6 +81,8 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
     # Make sure our file is backed up, but only back it up once per transaction.
     # We cheat and rely on the fact that @records is created on each prefetch.
     def self.backup_target(target)
+        return nil unless target_object(target).respond_to?(:backup)
+
         unless defined?(@backup_stats)
             @backup_stats = {}
         end
diff --git a/lib/puppet/provider/service/launchd.rb b/lib/puppet/provider/service/launchd.rb
index 891c96bd8..64b61ff85 100644
--- a/lib/puppet/provider/service/launchd.rb
+++ b/lib/puppet/provider/service/launchd.rb
@@ -103,12 +103,21 @@ Puppet::Type.type(:service).provide :launchd, :parent => :base do
 
     def status
         # launchctl list <jobname> exits zero if the job is loaded
-        # and non-zero if it isn't. Simple way to check...
+        # and non-zero if it isn't. Simple way to check... but is only
+        # available on OS X 10.5 unfortunately, so we grab the whole list
+        # and check if our resource is included. The output formats differ
+        # between 10.4 and 10.5, thus the necessity for splitting
         begin
-            launchctl :list, resource[:name]           
-            return :running
-        rescue Puppet::ExecutionFailure
+            output = launchctl :list
+            if output.nil?
+                raise Puppet::Error.new("launchctl list failed to return any data.")
+            end
+            output.split("\n").each do |j|
+                return :running if j.split(/\s/).last == resource[:name]
+            end
             return :stopped
+        rescue Puppet::ExecutionFailure
+            raise Puppet::Error.new("Unable to determine status of #{resource[:name]}")
         end
     end
 
diff --git a/lib/puppet/rails/resource.rb b/lib/puppet/rails/resource.rb
index bd2739d53..c3d287af2 100644
--- a/lib/puppet/rails/resource.rb
+++ b/lib/puppet/rails/resource.rb
@@ -1,5 +1,6 @@
 require 'puppet'
 require 'puppet/rails/param_name'
+require 'puppet/rails/param_value'
 require 'puppet/rails/puppet_tag'
 require 'puppet/util/rails/collection_merger'
 
@@ -57,7 +58,7 @@ class Puppet::Rails::Resource < ActiveRecord::Base
 
     # returns a hash of param_names.name => [param_values]
     def get_params_hash(values = nil)
-        values ||= @params_hash || Puppet::Rails::ParamValues.find_all_params_from_resource(id)
+        values ||= @params_hash || Puppet::Rails::ParamValue.find_all_params_from_resource(self)
         if values.size == 0
             return {}
         end
@@ -69,7 +70,7 @@ class Puppet::Rails::Resource < ActiveRecord::Base
     end
 
     def get_tag_hash(tags = nil)
-        tags ||= @tags_hash || Puppet::Rails::ResourceTag.find_all_tags_from_resource(id)
+        tags ||= @tags_hash || Puppet::Rails::ResourceTag.find_all_tags_from_resource(self)
         return tags.inject({}) do |hash, tag|
             # We have to store the tag object, not just the tag name.
             hash[tag['name']] = tag
@@ -99,7 +100,7 @@ class Puppet::Rails::Resource < ActiveRecord::Base
         result = get_params_hash
         result.each do |param, value|
             if value.is_a?(Array)
-                result[param] = value.collect { |v| v.value }
+                result[param] = value.collect { |v| v['value'] }
             else
                 result[param] = value.value
             end
diff --git a/lib/puppet/type/user.rb b/lib/puppet/type/user.rb
index 074afb6b2..8efc2ef1c 100755
--- a/lib/puppet/type/user.rb
+++ b/lib/puppet/type/user.rb
@@ -145,7 +145,7 @@ module Puppet
             desc "The user's password, in whatever encrypted format the local machine requires. Be sure to enclose any value that includes a dollar sign ($) in single quotes (\')."
 
             validate do |value|
-                raise ArgumentError, "Passwords cannot include ':'" if value.include?(":")
+                raise ArgumentError, "Passwords cannot include ':'" if value.is_a?(String) and value.include?(":")
             end
 
             def change_to_s(currentvalue, newvalue)
diff --git a/lib/puppet/util/filetype.rb b/lib/puppet/util/filetype.rb
index 0f184d5c2..34feeab90 100755
--- a/lib/puppet/util/filetype.rb
+++ b/lib/puppet/util/filetype.rb
@@ -72,11 +72,6 @@ class Puppet::Util::FileType
         @filetypes[type]
     end
 
-    # Back the file up before replacing it.
-    def backup
-        bucket.backup(@path) if File.exists?(@path)
-    end
-
     # Pick or create a filebucket to use.
     def bucket
         unless defined?(@bucket)
@@ -92,6 +87,11 @@ class Puppet::Util::FileType
 
     # Operate on plain files.
     newfiletype(:flat) do
+        # Back the file up before replacing it.
+        def backup
+            bucket.backup(@path) if File.exists?(@path)
+        end
+
         # Read the file.
         def read
             if File.exist?(@path)
diff --git a/spec/unit/network/xmlrpc/client.rb b/spec/unit/network/xmlrpc/client.rb
index 36e59429c..328688bae 100755
--- a/spec/unit/network/xmlrpc/client.rb
+++ b/spec/unit/network/xmlrpc/client.rb
@@ -109,7 +109,7 @@ describe Puppet::Network::XMLRPCClient do
             it "should log, recycle the connection, and retry if Errno::EPIPE is raised" do
                 @client.expects(:call).times(2).raises(Errno::EPIPE).then.returns "eh"
 
-                Puppet.expects(:warning)
+                Puppet.expects(:info)
                 @client.expects(:recycle_connection)
 
                 @client.report("eh")
@@ -118,7 +118,7 @@ describe Puppet::Network::XMLRPCClient do
             it "should log, recycle the connection, and retry if EOFError is raised" do
                 @client.expects(:call).times(2).raises(EOFError).then.returns "eh"
 
-                Puppet.expects(:warning)
+                Puppet.expects(:info)
                 @client.expects(:recycle_connection)
 
                 @client.report("eh")
diff --git a/spec/unit/provider/parsedfile.rb b/spec/unit/provider/parsedfile.rb
index 11a91c8d7..f20b6b235 100755
--- a/spec/unit/provider/parsedfile.rb
+++ b/spec/unit/provider/parsedfile.rb
@@ -56,18 +56,27 @@ describe Puppet::Provider::ParsedFile do
             @class.initvars
             @class.prefetch
 
-            @filetype = mock 'filetype'
-            Puppet::Util::FileType.filetype(:flat).expects(:new).with("/my/file").returns @filetype
+            @filetype = Puppet::Util::FileType.filetype(:flat).new("/my/file")
+            Puppet::Util::FileType.filetype(:flat).stubs(:new).with("/my/file").returns @filetype
 
             @filetype.stubs(:write)
         end
 
-        it "should back up the file being written" do
+        it "should back up the file being written if the filetype can be backed up" do
             @filetype.expects(:backup)
 
             @class.flush_target("/my/file")
         end
 
+        it "should not try to back up the file if the filetype cannot be backed up" do
+            @filetype = Puppet::Util::FileType.filetype(:ram).new("/my/file")
+            Puppet::Util::FileType.filetype(:flat).expects(:new).returns @filetype
+
+            @filetype.stubs(:write)
+
+            @class.flush_target("/my/file")
+        end
+
         it "should not back up the file more than once between calls to 'prefetch'" do
             @filetype.expects(:backup).once
 
diff --git a/spec/unit/provider/service/launchd.rb b/spec/unit/provider/service/launchd.rb
index cc2dae190..f2d69a47b 100755
--- a/spec/unit/provider/service/launchd.rb
+++ b/spec/unit/provider/service/launchd.rb
@@ -62,9 +62,17 @@ describe provider_class do
     
     describe "when checking status" do
         it "should call the external command 'launchctl list' once" do
-            @provider.expects(:launchctl).with(:list, @resource[:name]).returns(:running).once
+            @provider.expects(:launchctl).with(:list).returns("rotating-strawberry-madonnas")
             @provider.status
         end
+        it "should return stopped if not listed in launchctl list output" do
+            @provider.stubs(:launchctl).with(:list).returns("rotating-strawberry-madonnas")
+            assert_equal @provider.status, :stopped
+        end
+        it "should return running if listed in launchctl list output" do
+            @provider.stubs(:launchctl).with(:list).returns(@joblabel)
+            assert_equal @provider.status, :running
+        end
     end
     
     describe "when starting the service" do
diff --git a/spec/unit/type/user.rb b/spec/unit/type/user.rb
index 2e8616ce8..5c46689d3 100755
--- a/spec/unit/type/user.rb
+++ b/spec/unit/type/user.rb
@@ -38,10 +38,6 @@ describe user do
         it "should have a valid provider" do
             user.new(:name => "foo").provider.class.ancestors.should be_include(Puppet::Provider)
         end
-
-        it "should fail if a ':' is included in the password" do
-            lambda { user.create(:name => "foo", :password => 'some:thing') }.should raise_error(Puppet::Error)
-        end
     end
 
     properties = [:ensure, :uid, :gid, :home, :comment, :shell, :password, :groups, :roles, :auths, :profiles, :project, :keys]
@@ -232,6 +228,14 @@ describe user do
         it "should not include the password in the change log when changing the password" do
             @password.change_to_s("other", "mypass").should_not be_include("mypass")
         end
+
+        it "should fail if a ':' is included in the password" do
+            lambda { @password.should = "some:thing" }.should raise_error(ArgumentError)
+        end
+
+        it "should allow the value to be set to :absent" do
+            lambda { @password.should = :absent }.should_not raise_error
+        end
     end
 
     describe "when manages_solaris_rbac is enabled" do
diff --git a/test/language/resource.rb b/test/language/resource.rb
index fa04117f1..c7ed1f019 100755
--- a/test/language/resource.rb
+++ b/test/language/resource.rb
@@ -170,44 +170,4 @@ class TestResource < PuppetTest::TestCase
 
         assert_nil(hash[:owner], "got a value for an undef parameter")
     end
-
-    # #643 - Make sure virtual defines result in virtual resources
-    def test_virtual_defines
-        parser = mkparser
-        define = parser.newdefine("yayness",
-            :code => resourcedef("file", varref("name"),
-                "mode" => "644"))
-
-        config = mkcompiler(parser)
-
-        res = mkresource :type => "yayness", :title => "foo", :params => {}, :scope => config.topscope
-        res.virtual = true
-
-        result = nil
-        assert_nothing_raised("Could not evaluate defined resource") do
-            result = res.evaluate
-        end
-
-        scope = res.scope
-        newres = scope.findresource("File[foo]")
-        assert(newres, "Could not find resource")
-
-        assert(newres.virtual?, "Virtual defined resource generated non-virtual resources")
-
-        # Now try it with exported resources
-        res = mkresource :type => "yayness", :title => "bar", :params => {}, :scope => config.topscope
-        res.exported = true
-
-        result = nil
-        assert_nothing_raised("Could not evaluate exported resource") do
-            result = res.evaluate
-        end
-
-        scope = res.scope
-        newres = scope.findresource("File[bar]")
-        assert(newres, "Could not find resource")
-
-        assert(newres.exported?, "Exported defined resource generated non-exported resources")
-        assert(newres.virtual?, "Exported defined resource generated non-virtual resources")
-    end
 end