Fixing #1764 - a property's 'sync' method is never considered a no-op.

*This is a behaviour change.*

If the property does not return an event name, then one is generated
based on the property name.

Previously, the 'sync' method could return nil and it would be considered
a noop, but if you need a noop, then you need to modify your 'insync?' method
to return 'true' in the noop cases.

Also modifying all of the builtin types that didn't handle this explicitly or
returned nil in 'sync'.  There should be no behaviour change in any of them.

Signed-off-by: Luke Kanies <luke@madstop.com>

7 files changed, 23 insertions, 76 deletions

diff --git a/lib/puppet/property.rb b/lib/puppet/property.rb
index 50a1b71de..e6d0704e6 100644
--- a/lib/puppet/property.rb
+++ b/lib/puppet/property.rb
@@ -485,7 +485,7 @@ class Property < Puppet::Parameter
             end
 
             # This doc will probably get overridden
-            @doc ||= "The basic property that the object should be in."
+            @doc ||= "The basic property that the resource should be in."
         end
 
         def self.inherited(sub)
diff --git a/lib/puppet/transaction/change.rb b/lib/puppet/transaction/change.rb
index e05c2592c..42c3a174f 100644
--- a/lib/puppet/transaction/change.rb
+++ b/lib/puppet/transaction/change.rb
@@ -53,12 +53,10 @@ class Puppet::Transaction::Change
         # The transaction catches any exceptions here.
         events = @property.sync
         if events.nil?
-            return nil
-        end
-
-        if events.is_a?(Array)
+            events = [(@property.name.to_s + "_changed").to_sym]
+        elsif events.is_a?(Array)
             if events.empty?
-                return nil
+                events = [(@property.name.to_s + "_changed").to_sym]
             end
         else
             events = [events]
diff --git a/lib/puppet/type/file/mode.rb b/lib/puppet/type/file/mode.rb
index 8674e0a88..ada1b5b47 100755
--- a/lib/puppet/type/file/mode.rb
+++ b/lib/puppet/type/file/mode.rb
@@ -103,22 +103,8 @@ module Puppet
         end
 
         def sync
-            unless @resource.stat(false)
-                stat = @resource.stat(true)
-
-                unless stat
-                    self.debug "File does not exist; cannot set mode"
-                    return nil
-                end
-            end
-
             mode = self.should
 
-            if mode == :absent
-                # This is really only valid for create states...
-                return nil
-            end
-
             begin
                 File.chmod(mode, @resource[:path])
             rescue => detail
diff --git a/lib/puppet/type/file/selcontext.rb b/lib/puppet/type/file/selcontext.rb
index 084cd3d2c..22e3080b1 100644
--- a/lib/puppet/type/file/selcontext.rb
+++ b/lib/puppet/type/file/selcontext.rb
@@ -45,20 +45,6 @@ module Puppet
         end
 
         def sync
-            unless @resource.stat(false)
-                stat = @resource.stat(true)
-                unless stat
-                    return nil
-                end
-            end
-
-            selcontext = self.should
-
-            if selcontext == :absent
-                # This is only valid for create states...
-                return nil
-            end
-
             self.set_selinux_context(@resource[:path], @should, name)
             return :file_changed
         end
diff --git a/lib/puppet/type/group.rb b/lib/puppet/type/group.rb
index cb11a60a4..29486d3f0 100755
--- a/lib/puppet/type/group.rb
+++ b/lib/puppet/type/group.rb
@@ -21,8 +21,8 @@ module Puppet
             for Mac OS X, NetInfo is used.  This is currently unconfigurable,
             but if you desperately need it to be so, please contact us."
 
-        newproperty(:ensure) do
-            desc "The basic state that the object should be in."
+        ensurable do
+            desc "Create or remove the group."
 
             newvalue(:present) do
                 provider.create
@@ -35,33 +35,6 @@ module Puppet
 
                 :group_removed
             end
-
-            # If they're talking about the thing at all, they generally want to
-            # say it should exist.
-            defaultto do
-                if @resource.managed?
-                    :present
-                else
-                    nil
-                end
-            end
-
-            def retrieve
-                return provider.exists? ? :present : :absent
-            end
-
-            # The default 'sync' method only selects among a list of registered
-            # values.
-            def sync
-                unless self.class.values
-                    self.devfail "No values defined for %s" %
-                        self.class.name
-                end
-
-                # Set ourselves to whatever our should value is.
-                self.set(self.should)
-            end
-
         end
 
         newproperty(:gid) do
diff --git a/lib/puppet/type/tidy.rb b/lib/puppet/type/tidy.rb
index c352ec85b..a3d994128 100755
--- a/lib/puppet/type/tidy.rb
+++ b/lib/puppet/type/tidy.rb
@@ -44,6 +44,11 @@ module Puppet
             end
 
             def insync?(is)
+                if File.lstat(file).ftype == "directory" and ! @resource[:rmdirs]
+                    self.debug "Not tidying directories"
+                    return true
+                end
+
                 if is.is_a?(Symbol)
                     if [:absent, :notidy].include?(is)
                         return true
@@ -100,20 +105,17 @@ module Puppet
                 file = @resource[:path]
                 case File.lstat(file).ftype
                 when "directory":
-                    if @resource[:rmdirs]
-                        subs = Dir.entries(@resource[:path]).reject { |d|
-                            d == "." or d == ".."
-                        }.length
-                        if subs > 0
-                            self.info "%s has %s children; not tidying" %
-                                [@resource[:path], subs]
-                            self.info Dir.entries(@resource[:path]).inspect
-                        else
-                            Dir.rmdir(@resource[:path])
-                        end
+                    # If 'rmdirs' is disabled, then we would have never
+                    # gotten to this method.
+                    subs = Dir.entries(@resource[:path]).reject { |d|
+                        d == "." or d == ".."
+                    }.length
+                    if subs > 0
+                        self.info "%s has %s children; not tidying" %
+                            [@resource[:path], subs]
+                        self.info Dir.entries(@resource[:path]).inspect
                     else
-                        self.debug "Not tidying directories"
-                        return nil
+                        Dir.rmdir(@resource[:path])
                     end
                 when "file":
                     @resource.handlebackup(file)
diff --git a/lib/puppet/type/user.rb b/lib/puppet/type/user.rb
index 929e45d14..637fe9b46 100755
--- a/lib/puppet/type/user.rb
+++ b/lib/puppet/type/user.rb
@@ -109,6 +109,8 @@ module Puppet
                 end
 
                 fail "Could not find group(s) %s" % @should.join(",") unless found
+
+                # Use the default event.
             end
         end