Rewriting the tests for the package resource type, fixing #930.

6 files changed, 78 insertions, 138 deletions

diff --git a/lib/puppet/parameter.rb b/lib/puppet/parameter.rb
index fae0587e1..31e009af5 100644
--- a/lib/puppet/parameter.rb
+++ b/lib/puppet/parameter.rb
@@ -289,9 +289,6 @@ class Puppet::Parameter
             raise Puppet::DevError, "No resource set for %s" % self.class.name
         end
 
-        # LAK 2007-05-09: Keep the @parent around for backward compatibility.
-        #@parent = @resource
-
         if ! self.metaparam? and klass = Puppet::Type.metaparamclass(self.class.name)
             setup_shadow(klass)
         end
@@ -378,36 +375,37 @@ class Puppet::Parameter
         vals = self.class.values
         regs = self.class.regexes
 
-        if regs.is_a? Hash # this is true on properties
-            regs = regs.keys
-        end
-        if vals.empty? and regs.empty?
-            # This parameter isn't using defined values to do its work.
-            return 
-        end
+        # this is true on properties
+        regs = regs.keys if regs.is_a?(Hash)
+
+        # This parameter isn't using defined values to do its work.
+        return if vals.empty? and regs.empty?
+
         newval = value
-        unless value.is_a?(Symbol)
-            newval = value.to_s.intern
-        end
+        newval = value.to_s.intern unless value.is_a?(Symbol)
 
-        unless vals.include?(newval) or
-            self.class.alias(newval) or
-            self.class.match?(value) # We match the string, not the symbol
-                str = "Invalid '%s' value %s. " %
-                    [self.class.name, value.inspect]
+        name = newval
 
-                unless vals.empty?
-                    str += "Valid values are %s. " % vals.join(", ")
-                end
+        unless vals.include?(newval) or name = self.class.alias(newval) or name = self.class.match?(value) # We match the string, not the symbol
+            str = "Invalid '%s' value %s. " %
+                [self.class.name, value.inspect]
 
-                unless regs.empty?
-                    str += "Valid values match %s." % regs.collect { |r|
-                        r.to_s
-                    }.join(", ")
-                end
+            unless vals.empty?
+                str += "Valid values are %s. " % vals.join(", ")
+            end
+
+            unless regs.empty?
+                str += "Valid values match %s." % regs.collect { |r|
+                    r.to_s
+                }.join(", ")
+            end
 
-                raise ArgumentError, str
+            raise ArgumentError, str
         end
+
+        # Now check for features.
+        name = name[0] if name.is_a?(Array) # This is true for regexes.
+        validate_features_per_value(name) if is_a?(Puppet::Property)
     end
 
     def remove
@@ -484,5 +482,12 @@ class Puppet::Parameter
     def to_s
         s = "Parameter(%s)" % self.name
     end
+
+    # Make sure that we've got all of the required features for a given value.
+    def validate_features_per_value(value)
+        if features = self.class.value_option(value, :required_features)
+            raise ArgumentError, "Provider must have features '%s' to set '%s' to '%s'" % [features, self.class.name, value] unless provider.satisfies?(features)
+        end
+    end
 end
 
diff --git a/lib/puppet/property.rb b/lib/puppet/property.rb
index 84620bfb6..f3d879ee2 100644
--- a/lib/puppet/property.rb
+++ b/lib/puppet/property.rb
@@ -67,9 +67,7 @@ class Property < Puppet::Parameter
 
     # Retrieve an option set when a value was defined.
     def self.value_option(name, option)
-        if option.is_a?(String)
-            option = symbolize(option)
-        end
+        option = option.to_sym
         if hash = @parameteroptions[name]
             hash[option]
         else
@@ -209,28 +207,28 @@ class Property < Puppet::Parameter
     # Figure out which event to return.
     def event(name, event = nil)
         if value_event = self.class.value_option(name, :event)
-            return value_event
-        else
-            if event and event.is_a?(Symbol)
-                if event == :nochange
-                    return nil
-                else
-                    return event
-                end
+            return value_event 
+        end
+
+        if event and event.is_a?(Symbol)
+            if event == :nochange
+                return nil
             else
-                if self.class.name == :ensure
-                    event = case self.should
-                    when :present: (@resource.class.name.to_s + "_created").intern
-                    when :absent: (@resource.class.name.to_s + "_removed").intern
-                    else
-                        (@resource.class.name.to_s + "_changed").intern
-                    end
-                else
-                    event = (@resource.class.name.to_s + "_changed").intern
-                end
+                return event
             end
         end
 
+        if self.class.name == :ensure
+            event = case self.should
+            when :present: (@resource.class.name.to_s + "_created").intern
+            when :absent: (@resource.class.name.to_s + "_removed").intern
+            else
+                (@resource.class.name.to_s + "_changed").intern
+            end
+        else
+            event = (@resource.class.name.to_s + "_changed").intern
+        end
+
         return event
     end
     
@@ -297,8 +295,7 @@ class Property < Puppet::Parameter
     # Send a log message.
     def log(msg)
         unless @resource[:loglevel]
-            self.devfail "Parent %s has no loglevel" %
-                @resource.name
+            self.devfail "Parent %s has no loglevel" % @resource.name
         end
         Puppet::Util::Log.create(
             :level => @resource[:loglevel],
@@ -338,10 +335,7 @@ class Property < Puppet::Parameter
     # provider.  In other words, if the property name is 'gid', we'll call
     # 'provider.gid' to retrieve the current value.
     def retrieve
-        is = provider.send(self.class.name)
-#        puts "IS is: " + is.to_s
-#       puts "and its an array!!!" if is.is_a? Array
-        return is
+        provider.send(self.class.name)
     end
 
     # Set our value, using the provider, an associated block, or both.
@@ -420,17 +414,9 @@ class Property < Puppet::Parameter
         end
     end
 
-    # The default 'sync' method only selects among a list of registered
-    # values.
+    # The default 'sync' method only selects among a list of registered # values.
     def sync
-#        if self.insync?
-#            self.info "already in sync"
-#            return nil
-#        end
-        unless self.class.values
-            self.devfail "No values defined for %s" %
-                self.class.name
-        end
+        self.devfail("No values defined for %s" % self.class.name) unless self.class.values
 
         if value = self.should
             set(value)
diff --git a/lib/puppet/provider.rb b/lib/puppet/provider.rb
index 26c1254e3..e73bb0cb6 100644
--- a/lib/puppet/provider.rb
+++ b/lib/puppet/provider.rb
@@ -64,6 +64,11 @@ class Puppet::Provider
         end
     end
 
+    # Is the provided feature a declared feature?
+    def self.declared_feature?(name)
+        defined?(@declared_features) and @declared_features.include?(name)
+    end
+
     # Does this implementation match all of the default requirements?  If
     # defaults are empty, we return false.
     def self.default?
diff --git a/lib/puppet/provider/package.rb b/lib/puppet/provider/package.rb
index 094bfb9f4..f7ff7e55a 100644
--- a/lib/puppet/provider/package.rb
+++ b/lib/puppet/provider/package.rb
@@ -27,4 +27,3 @@ class Puppet::Provider::Package < Puppet::Provider
         @property_hash.dup
     end
 end
-
diff --git a/lib/puppet/type/package.rb b/lib/puppet/type/package.rb
index e04f651a9..616362206 100644
--- a/lib/puppet/type/package.rb
+++ b/lib/puppet/type/package.rb
@@ -4,7 +4,6 @@
 # systems.
 
 module Puppet
-    class PackageError < Puppet::Error; end
     newtype(:package) do
         @doc = "Manage packages.  There is a basic dichotomy in package
             support right now:  Some package types (e.g., yum and apt) can
@@ -108,25 +107,15 @@ module Puppet
                 @lateststamp ||= (Time.now.to_i - 1000)
                 # Iterate across all of the should values, and see how they
                 # turn out.
+
                 @should.each { |should|
                     case should
                     when :present
-                        unless [:absent, :purged].include?(is)
-                            return true
-                        end
+                        return true unless [:absent, :purged].include?(is)
                     when :latest
                         # Short-circuit packages that are not present
-                        if is == :absent
-                            return false
-                        end
-
-                        unless provider.respond_to?(:latest)
-                            self.fail(
-                                "Package type %s does not support specifying 'latest'" %
-                                @resource[:provider]
-                            )
-                        end
-
+                        return false if is == :absent or is == :purged
+ 
                         # Don't run 'latest' more than about every 5 minutes
                         if @latest and ((Time.now.to_i - @lateststamp) / 60) < 5
                             #self.debug "Skipping latest check"
@@ -153,9 +142,9 @@ module Puppet
                                 [is.inspect, @resource.name, @latest.inspect]
                         end
                     when :absent
-                        if is == :absent or is == :purged
-                            return true
-                        end
+                        return true if is == :absent or is == :purged
+                    when :purged
+                        return true if is == :purged
                     when is
                         return true
                     end
@@ -312,46 +301,6 @@ module Puppet
             autos
         end
 
-        @listed = false
-
-        @allowedmethods = [:types]
-
-        class << self
-            attr_reader :listed
-        end
-
-        def self.clear
-            @listed = false
-            super
-        end
-
-        # Create a new package object from listed information
-        def self.installedpkg(hash)
-            unless hash.include? :provider
-                raise Puppet::DevError, "Got installed package with no provider"
-            end
-            # this is from code, so we don't have to do as much checking
-            name = hash[:name]
-            hash.delete(:name)
-
-            object = self[name] || self.create(:name => name)
-            object.setparams(hash)
-
-            return object
-        end
-
-        # Iterate across all packages of a given type and mark them absent
-        # if they are not in the list
-        def self.markabsent(pkgtype, packages)
-            # Mark any packages we didn't find as absent
-            self.each do |pkg|
-                next unless packages[:provider] == pkgtype
-                unless packages.include? pkg
-                    pkg.provider.send(:ensure, :absent)
-                end
-            end
-        end
-
         # This only exists for testing.
         def clear
             if obj = @parameters[:ensure]
@@ -365,10 +314,6 @@ module Puppet
             @provider.get(:ensure) != :absent
         end
 
-        # okay, there are two ways that a package could be created...
-        # either through the language, in which case the hash's values should
-        # be set in 'should', or through comparing against the system, in which
-        # case the hash's values should be set in 'is'
         def initialize(params)
             self.initvars
             provider = nil
diff --git a/lib/puppet/util/provider_features.rb b/lib/puppet/util/provider_features.rb
index d5f272420..3a9d4262d 100644
--- a/lib/puppet/util/provider_features.rb
+++ b/lib/puppet/util/provider_features.rb
@@ -55,9 +55,7 @@ module Puppet::Util::ProviderFeatures
     # required to determine if the feature is present.
     def feature(name, docs, hash = {})
         @features ||= {}
-        if @features.include?(name)
-            raise Puppet::DevError, "Feature %s is already defined" % name
-        end
+        raise(Puppet::DevError, "Feature %s is already defined" % name) if @features.include?(name)
         begin
             obj = ProviderFeature.new(name, docs, hash)
             @features[obj.name] = obj
@@ -133,7 +131,8 @@ module Puppet::Util::ProviderFeatures
                 }
             end
 
-            # Create a method that will list all functional features.
+            # Create a method that will determine if a provided list of
+            # features are satisfied by the curred provider.
             @feature_module.send(:define_method, :satisfies?) do |*needed|
                 ret = true
                 needed.flatten.each do |feature|
@@ -150,13 +149,7 @@ module Puppet::Util::ProviderFeatures
             @features.each do |name, feature|
                 method = name.to_s + "?"
                 @feature_module.send(:define_method, method) do
-                    if defined? @declared_features and @declared_features.include?(name)
-                        true
-                    elsif feature.available?(self)
-                        true
-                    else
-                        false
-                    end
+                    self.class.declared_feature?(name) or feature.available?(self)
                 end
             end
 
@@ -173,5 +166,12 @@ module Puppet::Util::ProviderFeatures
         end
         @feature_module
     end
+
+    # Return the actual provider feature instance.  Really only used for testing.
+    def provider_feature(name)
+        return nil unless defined?(@features)
+
+        @features[name]
+    end
 end