Users and groups now work on OS X. I had to make some key changes to how transactions and state changes work -- the most important is that it is no longer an error to try to sync a state that is already in sync. I could not find another way to handle all user states being out of sync but the first state actually syncs everything.

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

7 files changed, 72 insertions, 42 deletions

diff --git a/lib/puppet/statechange.rb b/lib/puppet/statechange.rb
index cd5195616..0c817e909 100644
--- a/lib/puppet/statechange.rb
+++ b/lib/puppet/statechange.rb
@@ -8,7 +8,7 @@
 
 module Puppet
 	class StateChange
-        attr_accessor :is, :should, :type, :path, :state, :transaction, :run
+        attr_accessor :is, :should, :type, :path, :state, :transaction, :changed
 
 		#---------------------------------------------------------------
         def initialize(state)
@@ -23,7 +23,7 @@ module Puppet
             end
             @should = state.should
 
-            @run = false
+            @changed = false
         end
 		#---------------------------------------------------------------
 
@@ -35,9 +35,9 @@ module Puppet
             end
 
             if @state.is == @state.should
-                raise Puppet::Error.new(
-                    "Tried to change in-sync state %s" % state.name
-                )
+                Puppet.info "%s.%s is already in sync" %
+                    [@state.parent.name, @state.name]
+                return nil
             end
 
             begin
@@ -46,10 +46,13 @@ module Puppet
                     return nil
                 end
 
-                unless events.is_a?(Array)
+                if events.is_a?(Array)
+                    if events.empty?
+                        return nil
+                    end
+                else
                     events = [events]
                 end
-                @run = true
                 
                 return events.collect { |event|
                     # default to a simple event type
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index b44393efd..59e106902 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -67,14 +67,16 @@ class Transaction
                     # should do so
                 end
 
-                if events.nil?
-                    Puppet.debug "No events returned?"
+                unless events.nil? or (events.is_a?(Array) and events.empty?)
+                    change.changed = true
                 end
                 events
             elsif change.is_a?(Puppet::Transaction)
+                raise Puppet::DevError, "Got a sub-transaction"
                 change.evaluate
             else
-                raise "Transactions cannot handle objects of type %s" % child.class
+                raise Puppet::DevError,
+                    "Transactions cannot handle objects of type %s" % child.class
             end
         }.flatten.reject { |event|
             event.nil?
@@ -124,7 +126,10 @@ class Transaction
         events = @changes.reverse.collect { |change|
             if change.is_a?(Puppet::StateChange)
                 # skip changes that were never actually run
-                next unless change.run
+                unless change.changed
+                    Puppet.debug "%s was not changed" % change.to_s
+                    next
+                end
                 #change.transaction = self
                 begin
                     change.backward
@@ -140,10 +145,12 @@ class Transaction
                     # but a chmod failed?  how would i handle that error? dern
                 end
             elsif change.is_a?(Puppet::Transaction)
+                raise Puppet::DevError, "Got a sub-transaction"
                 # yay, recursion
                 change.rollback
             else
-                raise "Transactions cannot handle objects of type %s" % child.class
+                raise Puppe::DevError,
+                    "Transactions cannot handle objects of type %s" % child.class
             end
         }.flatten.reject { |e| e.nil? }
 
diff --git a/lib/puppet/type/group.rb b/lib/puppet/type/group.rb
index eb15907af..4da1b94b1 100755
--- a/lib/puppet/type/group.rb
+++ b/lib/puppet/type/group.rb
@@ -46,10 +46,6 @@ module Puppet
                     else
                         raise Puppet::Error, "Invalid GID %s" % gid
                     end
-                when Integer
-                    unless gid >= 0
-                        raise Puppet::Error, "GIDs must be positive"
-                    end
                 when Symbol
                     unless gid == :auto or gid == :notfound
                         raise Puppet::DevError, "Invalid GID %s" % gid
diff --git a/lib/puppet/type/nameservice/netinfo.rb b/lib/puppet/type/nameservice/netinfo.rb
index 35d7348d8..7fde579e7 100644
--- a/lib/puppet/type/nameservice/netinfo.rb
+++ b/lib/puppet/type/nameservice/netinfo.rb
@@ -67,6 +67,7 @@ module Puppet
                 end
 
                 def retrieve
+                    NetInfo.flush
                     dir = @parent.class.netinfodir
                     cmd = ["nireport", "/", "/%s" % dir, "name"]
 
diff --git a/lib/puppet/type/nameservice/posix.rb b/lib/puppet/type/nameservice/posix.rb
index 33d364dda..af62b72ce 100644
--- a/lib/puppet/type/nameservice/posix.rb
+++ b/lib/puppet/type/nameservice/posix.rb
@@ -97,6 +97,10 @@ module Puppet
 
                 def sync
                     event = nil
+                    # they're in sync some other way
+                    if @is == @should
+                        return nil
+                    end
                     if @is == :notfound
                         self.retrieve
                         if @is == @should
@@ -108,11 +112,12 @@ module Puppet
                     if @is == :notfound or @should == :notfound
                         event = syncname()
 
+                        return event
                         # if the whole object is created at once, just return
                         # an event saying so
-                        if self.class.allatonce?
-                            return event
-                        end
+                        #if self.class.allatonce?
+                        #    return event
+                        #end
                     end
 
                     unless @parent.exists?
@@ -180,10 +185,9 @@ module Puppet
                     # not many
                     unless self.class.allatonce?
                         if type == "create"
-                            Puppet.info "syncing everyone"
                             @parent.eachstate { |state|
-                                Puppet.info "syncing %s" % state.name
                                 state.sync
+                                state.retrieve
                             }
                         end
                     end
diff --git a/lib/puppet/type/user.rb b/lib/puppet/type/user.rb
index c0e82a2e3..b88bc5c5a 100755
--- a/lib/puppet/type/user.rb
+++ b/lib/puppet/type/user.rb
@@ -72,10 +72,6 @@ module Puppet
                     else
                         method = :getgrnam
                     end
-                when Integer
-                    unless gid >= 0
-                        raise Puppet::Error, "GIDs must be positive"
-                    end
                 when Symbol
                     unless gid == :auto or gid == :notfound
                         raise Puppet::DevError, "Invalid GID %s" % gid
diff --git a/test/types/tc_user.rb b/test/types/tc_user.rb
index 057cd40e0..7c82587c7 100755
--- a/test/types/tc_user.rb
+++ b/test/types/tc_user.rb
@@ -109,6 +109,21 @@ class TestUser < TestPuppet
         }
     end
 
+    def mkuser(name)
+        user = nil
+        assert_nothing_raised {
+            user = Puppet::Type::User.create(
+                :name => name,
+                :comment => "Puppet Testing User",
+                :gid => Process.gid,
+                :shell => findshell(),
+                :home => "/home/%s" % name
+            )
+        }
+
+        return user
+    end
+
     def attrtest_comment(user)
         old = user.is(:comment)
         user[:comment] = "A different comment"
@@ -179,8 +194,6 @@ class TestUser < TestPuppet
         comp = newcomp("gidtest", user)
 
         user.retrieve
-        Puppet.notice "%s vs %s vs %s" %
-            [user.is(:gid), user.should(:gid), old.inspect]
 
         user[:gid] = old
 
@@ -269,9 +282,10 @@ class TestUser < TestPuppet
 
         Puppet::Type::User.validstates.each { |name, state|
             assert_nothing_raised {
-                method = state.infomethod
+                method = state.posixmethod
                 assert(method, "State %s has no infomethod" % name)
-                assert(obj.respond_to?(method), "State %s has an invalid method %s" %
+                assert(obj.respond_to?(method),
+                    "State %s has an invalid method %s" %
                     [name, method])
             }
         }
@@ -279,20 +293,33 @@ class TestUser < TestPuppet
 
     if Process.uid == 0
         def test_simpleuser
+            name = "pptest"
+
+            assert(missing?(name), "User %s is present" % name)
+
+            user = mkuser(name)
+
+            @@tmpusers << name
+
+            comp = newcomp("usercomp", user)
+
+            trans = assert_events(comp, [:user_created], "user")
+
+            assert_equal("Puppet Testing User", current?(:comment, user[:name]),
+                "Comment was not set")
+
+            assert_rollback_events(trans, [:user_deleted], "user")
+
+            assert(missing?(user[:name]))
+        end
+
+        def test_allstates
             user = nil
             name = "pptest"
 
             assert(missing?(name), "User %s is present" % name)
 
-            assert_nothing_raised {
-                user = Puppet::Type::User.create(
-                    :name => name,
-                    :comment => "Puppet Testing User",
-                    :gid => Process.gid,
-                    :shell => findshell(),
-                    :home => "/home/%s" % name
-                )
-            }
+            user = mkuser(name)
 
             @@tmpusers << name
 
@@ -315,10 +342,6 @@ class TestUser < TestPuppet
                     $stderr.puts "Not testing attr %s of user" % test
                 end
             }
-
-            assert_rollback_events(trans, [:user_deleted], "user")
-
-            assert(missing?(user[:name]))
         end
     else
         $stderr.puts "Not root; skipping user creation/modification tests"