Cron is now fully functional and tested on 3 platforms. In order to make it work, I had to do some modifications to TransObject#to_type and Type.create, but all tests pass now. Type.create is now handling errors on creating objects, so if you try to create an invalid object you will just get nil returned, rather than receiving an error.

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

5 files changed, 271 insertions, 104 deletions

diff --git a/lib/puppet/transportable.rb b/lib/puppet/transportable.rb
index 57e5f8377..9d1e85147 100644
--- a/lib/puppet/transportable.rb
+++ b/lib/puppet/transportable.rb
@@ -52,23 +52,7 @@ module Puppet
         def to_type
             retobj = nil
             if type = Puppet::Type.type(self.type)
-                begin
-                    retobj = type.create(self)
-                rescue => detail
-                    # FIXME TransObject should be handling what happens when there's an error
-                    if Puppet[:debug]
-                        if detail.respond_to?(:stack)
-                            puts detail.stack
-                        end
-                    end
-                    Puppet.err "Could not create %s: %s" % [self[:name], detail.to_s]
-                    if retobj
-                        retobj.destroy()
-                    else
-                        if obj = type[self[:name]]
-                            obj.destroy()
-                        end
-                    end
+                unless retobj = type.create(self)
                     return nil
                 end
                 retobj.file = @file
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 4826686b4..49190596a 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -499,6 +499,10 @@ class Type < Puppet::Element
         @dependencies.each { |dep|
             dep.unsubscribe(self)
         }
+
+        if defined? @parent and @parent
+            @parent.delete(self)
+        end
     end
     #---------------------------------------------------------------
 
@@ -681,18 +685,38 @@ class Type < Puppet::Element
     public
 
     #---------------------------------------------------------------
-    # force users to call this, so that we can merge objects if
-    # necessary
+    # Force users to call this, so that we can merge objects if
+    # necessary.  FIXME This method should be responsible for most of the
+    # error handling.
     def self.create(hash)
         if name =   hash["name"] || hash[:name] ||
                     hash[self.namevar] || hash[self.namevar.to_s]
             # if the object already exists
             if retobj = self[name]
+                # merge the new data
                 retobj.merge(hash)
 
                 return retobj
             else
-                return new(hash)
+                # create it anew
+                # if there's a failure, destroy the object if it got that far
+                begin
+                    obj = new(hash)
+                rescue => detail
+                    if Puppet[:debug]
+                        if detail.respond_to?(:stack)
+                            puts detail.stack
+                        end
+                    end
+                    Puppet.err "Could not create %s: %s" % [name, detail.to_s]
+                    if obj
+                        Puppet.err obj
+                        obj.destroy
+                    elsif obj = self[name]
+                        obj.destroy
+                    end
+                    return nil
+                end
             end
         else
             raise Puppet::Error, "You must specify a name for objects of type %s" %
@@ -1308,7 +1332,7 @@ end
 
 require 'puppet/statechange'
 require 'puppet/type/component'
-#require 'puppet/type/cron'
+require 'puppet/type/cron'
 require 'puppet/type/exec'
 require 'puppet/type/group'
 require 'puppet/type/package'
diff --git a/lib/puppet/type/component.rb b/lib/puppet/type/component.rb
index 345afb75b..a81f62deb 100644
--- a/lib/puppet/type/component.rb
+++ b/lib/puppet/type/component.rb
@@ -52,6 +52,15 @@ module Puppet
             end
         end
 
+        def delete(child)
+            if @children.include?(child)
+                @children.delete(child)
+                return true
+            else
+                return false
+            end
+        end
+
         def each
             @children.each { |child| yield child }
         end
diff --git a/lib/puppet/type/cron.rb b/lib/puppet/type/cron.rb
index 09d50c813..6f8b98367 100755
--- a/lib/puppet/type/cron.rb
+++ b/lib/puppet/type/cron.rb
@@ -1,72 +1,84 @@
-#!/usr/local/bin/ruby -w
-
-# $Id$
+# A Puppet::Type class to manage cron jobs.  Some abstraction is done,
+# so that different platforms' versions of +crontab+ all work equivalently.
 
+require 'etc'
 require 'facter'
 require 'puppet/type/state'
 
 module Puppet
+    # The Puppet::CronType modules are responsible for the actual abstraction.
+    # They must implement three module functions: +read+, +write+, and +remove+,
+    # analogous to the three flags accepted by most implementations of +crontab+.
+    # All of these methods require the user name to be passed in.
+    #
+    # These modules operate on the strings that are ore become the cron tabs --
+    # they do not have any semantic understanding of what they are reading or
+    # writing.
     module CronType
+        # Retrieve the uid of a user.  This is duplication of code, but the unless
+        # I start using Puppet::Type::User objects here, it's a much better design.
+        def self.uid(user)
+            begin
+                return Etc.getpwnam(user).uid
+            rescue ArgumentError
+                raise Puppet::Error, "User %s not found" % user
+            end
+        end
+
+        # This module covers nearly everyone; SunOS is only known exception so far.
         module Default
+            # Only add the -u flag when the user is different.  Fedora apparently
+            # does not think I should be allowed to set the user to myself.
+            def self.cmdbase(user)
+                uid = CronType.uid(user)
+                cmd = nil
+                if uid == Process.uid
+                    return "crontab"
+                else
+                    return "crontab -u #{user}"
+                end
+            end
+
+            # Read a specific user's cron tab.
             def self.read(user)
-                tab = %x{crontab -u #{user} -l 2>/dev/null}
+                tab = %x{#{self.cmdbase(user)} -l 2>/dev/null}
             end
 
+            # Remove a specific user's cron tab.
             def self.remove(user)
-                %x{crontab -u #{user} -r 2>/dev/null}
+                %x{#{self.cmdbase(user)} -r 2>/dev/null}
             end
 
+            # Overwrite a specific user's cron tab; must be passed the user name
+            # and the text with which to create the cron tab.
             def self.write(user, text)
-                IO.popen("crontab -u #{user} -", "w") { |p|
+                IO.popen("#{self.cmdbase(user)} -", "w") { |p|
                     p.print text
                 }
             end
         end
 
+        # SunOS has completely different cron commands; this module implements
+        # its versions.
         module SunOS
-            def self.asuser(user)
-                # FIXME this should use our user object, since it already does
-                # this for us
-                require 'etc'
-
-                begin
-                    obj = Etc.getpwnam(user)
-                rescue ArgumentError
-                    raise Puppet::Error, "User %s not found"
-                end
-
-                uid = obj.uid
-
-                olduid = nil
-                if Process.uid == uid
-                    olduid = Process.uid
-                    Process.euid = uid
-                end
-
-                retval = yield
-
-
-                if olduid
-                    Process.euid = olduid
-                end
-
-                return retval
-            end
-
+            # Read a specific user's cron tab.
             def self.read(user)
-                self.asuser(user) {
+                Puppet.asuser(user) {
                     %x{crontab -l 2>/dev/null}
                 }
             end
 
+            # Remove a specific user's cron tab.
             def self.remove(user)
-                self.asuser(user) {
+                Puppet.asuser(user) {
                     %x{crontab -r 2>/dev/null}
                 }
             end
 
+            # Overwrite a specific user's cron tab; must be passed the user name
+            # and the text with which to create the cron tab.
             def self.write(user, text)
-                self.asuser(user) {
+                Puppet.asuser(user) {
                     IO.popen("crontab", "w") { |p|
                         p.print text
                     }
@@ -76,6 +88,13 @@ module Puppet
     end
 
     class State
+        # This is Cron's single state.  Somewhat uniquely, this state does not
+        # actually change anything -- it just calls +@parent.sync+, which writes
+        # out the whole cron tab for the user in question.  There is no real way
+        # to change individual cron jobs without rewriting the entire cron file.
+        #
+        # Note that this means that managing many cron jobs for a given user
+        # could currently result in multiple write sessions for that user.
         class CronCommand < Puppet::State
             @name = :command
             @doc = "The command to execute in the cron job.  The environment
@@ -84,12 +103,18 @@ module Puppet
                 profile is not sourced when the command is run, so if the
                 user's environment is desired it should be sourced manually."
 
+            # Normally this would retrieve the current value, but our state is not
+            # actually capable of doing so.  The Cron class does the actual tab
+            # retrieval, so all this method does is default to :notfound for @is.
             def retrieve
                 unless defined? @is and ! @is.nil?
                     @is = :notfound
                 end
             end
 
+            # Determine whether the cron job should be destroyed, and figure
+            # out which event to return.  Finally, call @parent.sync to write the
+            # cron tab.
             def sync
                 @parent.store
 
@@ -99,6 +124,7 @@ module Puppet
                     event = :cron_created
                 elsif @should == :notfound
                     # FIXME I need to actually delete the cronjob...
+                    @parent.destroy
                     event = :cron_deleted
                 elsif @should == @is
                     Puppet.err "Uh, they're both %s" % @should
@@ -117,6 +143,10 @@ module Puppet
     end
 
     class Type
+        # Model the actual cron jobs.  Supports all of the normal cron job fields
+        # as parameters, with the 'command' as the single state.  Also requires a
+        # completely symbolic 'name' paremeter, which gets written to the file
+        # and is used to manage the job.
         class Cron < Type
             @states = [
                 Puppet::State::CronCommand
@@ -149,10 +179,15 @@ module Puppet
             @paramdoc[:monthday] = "The day of the month on which to run the
                 command.  Optional; if specified, must be between 1 and 31."
 
-            @doc = "Installs cron jobs.  All fields except the command 
+            @doc = "Installs and manages cron jobs.  All fields except the command 
                 and the user are optional, although specifying no periodic
                 fields would result in the command being executed every
-                minute."
+                minute.  While the name of the cron job is not part of the actual
+                job, it is used by Puppet to store and retrieve it.  If you specify
+                a cron job that matches an existing job in every way except name,
+                then the jobs will be considered equivalent and the new name will
+                be permanently associated with that job.  Once this association is
+                made and synced to disk, you can then manage the job normally."
             @name = :cron
             @namevar = :name
 
@@ -174,9 +209,21 @@ module Puppet
                 @crontype = Puppet::CronType::Default
             end
 
-            # FIXME so the fundamental problem is, what if the object
-            # already exists?
+            class << self
+                attr_accessor :crontype
+            end
+
+            attr_accessor :uid
+
+            # Override the Puppet::Type#[]= method so that we can store the instances
+            # in per-user arrays.  Then just call +super+.
+            def self.[]=(name, object)
+                self.instance(object)
+                super
+            end
 
+            # In addition to removing the instances in @objects, Cron has to remove
+            # per-user cron tab information.
             def self.clear
                 @instances = {}
                 @loaded = {}
@@ -184,14 +231,32 @@ module Puppet
                 super
             end
 
-            def self.crontype
-                return @crontype
+            # Override the default Puppet::Type method, because instances
+            # also need to be deleted from the @instances hash
+            def self.delete(child)
+                if @instances.include?(child[:user])
+                    if @instances[child[:user]].include?(child)
+                        @instances[child[:user]].delete(child)
+                    end
+                end
+                super
             end
 
+            # Return the fields found in the cron tab.
             def self.fields
                 return [:minute, :hour, :monthday, :month, :weekday, :command]
             end
 
+            # Return the header placed at the top of each generated file, warning
+            # users that modifying this file manually is probably a bad idea.
+            def self.header
+%{#This file was autogenerated at #{Time.now} by puppet.  While it
+# can still be managed manually, it is definitely not recommended.
+# Note particularly that the comments starting with 'Puppet Name' should
+# not be deleted, as doing so could cause duplicate cron jobs.\n}
+            end
+
+            # Store a new instance of a cron job.  Called from Cron#initialize.
             def self.instance(obj)
                 user = obj[:user]
                 if @instances.include?(user)
@@ -203,14 +268,22 @@ module Puppet
                 end
             end
 
-            def self.retrieve(user)
+            # Parse a user's cron job into individual cron objects.
+            #
+            # Autogenerates names for any jobs that don't already have one; these
+            # names will get written back to the file.
+            #
+            # This method also stores existing comments, and it stores all cron
+            # jobs in order, mostly so that comments are retained in the order
+            # they were written and in proximity to the same jobs.
+            def self.parse(user, text)
+                count = 0
                 hash = {}
                 name = nil
                 unless @instances.include?(user)
                     @instances[user] = []
                 end
-                #%x{crontab -u #{user} -l 2>/dev/null}.split("\n").each { |line|
-                @crontype.read(user).split("\n").each { |line|
+                text.chomp.split("\n").each { |line|
                     case line
                     when /^# Puppet Name: (\w+)$/: name = $1
                     when /^#/:
@@ -237,7 +310,26 @@ module Puppet
                         unless hash.include?(:command)
                             raise Puppet::DevError, "No command for %s" % name
                         end
-                        unless cron = Puppet::Type::Cron[hash[:command]]
+                        # if the cron already exists with that name...
+                        if cron = Puppet::Type::Cron[hash[:command]]
+                            # do nothing...
+                        elsif tmp = @instances[user].reject { |obj|
+                                    ! obj.is_a?(Cron)
+                                }.find { |obj|
+                                    obj.should(:command) == hash[:command]
+                                }
+                            # if we can find a cron whose spec exactly matches
+
+                            # we now have a cron job whose command exactly matches
+                            # let's see if the other fields match
+                            txt = tmp.to_cron.sub(/#.+\n/,'')
+
+                            if txt == line
+                                cron = tmp
+                            end
+                        else
+                            # create a new cron job, since no existing one
+                            # seems to match
                             cron = Puppet::Type::Cron.create(
                                 :name => name
                             )
@@ -248,34 +340,57 @@ module Puppet
                         }
                         hash.clear
                         name = nil
+                        count += 1
                     end
                 }
-                if $? == 0
-                    #return tab
+            end
+
+            # Retrieve a given user's cron job, using the @crontype's +retrieve+
+            # method.  Returns nil if there was no cron job; else, returns the
+            # number of cron instances found.
+            def self.retrieve(user)
+                #%x{crontab -u #{user} -l 2>/dev/null}.split("\n").each { |line|
+                text = @crontype.read(user)
+                if $? != 0
+                    # there is no cron file
+                    return nil
                 else
-                    #return nil
+                    self.parse(user, text)
                 end
 
                 @loaded[user] = Time.now
             end
 
+            # Store the user's cron tab.  Collects the text of the new tab and
+            # sends it to the +@crontype+ module's +write+ function.  Also adds
+            # header warning users not to modify the file directly.
             def self.store(user)
                 if @instances.include?(user)
-                    @crontype.write(user,
-                        @instances[user].collect { |obj|
-                            if obj.is_a?(Cron)
-                                obj.to_cron
-                            else
-                                obj.to_s
-                            end
-                        }.join("\n")
-                    )
+                    @crontype.write(user, self.header + self.tab(user))
                     @synced[user] = Time.now
                 else
                     Puppet.notice "No cron instances for %s" % user
                 end
             end
 
+            # Collect all Cron instances for a given user and convert them
+            # into literal text.
+            def self.tab(user)
+                if @instances.include?(user)
+                    return @instances[user].collect { |obj|
+                        if obj.is_a?(Cron)
+                            obj.to_cron
+                        else
+                            obj.to_s
+                        end
+                    }.join("\n") + "\n"
+                else
+                    Puppet.notice "No cron instances for %s" % user
+                end
+            end
+
+            # Return the last time a given user's cron tab was loaded.  Could
+            # be used for reducing writes, but currently is not.
             def self.loaded?(user)
                 if @loaded.include?(user)
                     return @loaded[user]
@@ -284,11 +399,10 @@ module Puppet
                 end
             end
 
-            def initialize(hash)
-                super
-                self.class.instance(self)
-            end
-
+            # Because the current value is retrieved by the +@crontype+ module,
+            # the +is+ value on the state is set by an outside party.  Cron is
+            # currently the only class that needs this, so this method is provided
+            # specifically for it.
             def is=(ary)
                 param, value = ary
                 if param.is_a?(String)
@@ -302,6 +416,9 @@ module Puppet
                 end
             end
 
+            # A method used to do parameter input handling.  Converts integers in
+            # string form to actual integers, and returns the value if it's an integer
+            # or false if it's just a normal string.
             def numfix(num)
                 if num =~ /^\d+$/
                     return num.to_i
@@ -312,7 +429,9 @@ module Puppet
                 end
             end
 
-            def limitcheck(num, lower, upper, type)
+            # Verify that a number is within the specified limits.  Return the
+            # number if it is, or false if it is not.
+            def limitcheck(num, lower, upper)
                 if num >= lower and num <= upper
                     return num
                 else
@@ -320,7 +439,10 @@ module Puppet
                 end
             end
 
-            def alphacheck(value, type, ary)
+            # Verify that a value falls within the specified array.  Does case
+            # insensitive matching, and supports matching either the entire word
+            # or the first three letters of the word.
+            def alphacheck(value, ary)
                 tmp = value.downcase
                 if tmp.length == 3
                     ary.each_with_index { |name, index|
@@ -337,20 +459,34 @@ module Puppet
                 return false
             end
 
-            def parameter(value, type, lower, upper, alpha = nil, ary = nil)
-                retval = nil
-                if num = numfix(value)
-                    retval = limitcheck(num, lower, upper, type)
-                elsif alpha
-                    retval = alphacheck(value, type, ary)
+            # The method that does all of the actual parameter value checking; called
+            # by all of the +param<name>=+ methods.  Requires the value, type, and
+            # bounds, and optionally supports a boolean of whether to do alpha
+            # checking, and if so requires the ary against which to do the checking.
+            def parameter(values, type, lower, upper, alpha = nil, ary = nil)
+                unless values.is_a?(Array)
+                    if values =~ /,/
+                        values = values.split(/,/)
+                    else
+                        values = [values]
+                    end
                 end
 
-                if retval
-                    @parameters[type] = retval
-                else
-                    raise Puppet::Error, "%s is not a valid %s" %
-                        [value, type]
-                end
+                @parameters[type] = values.collect { |value|
+                    retval = nil
+                    if num = numfix(value)
+                        retval = limitcheck(num, lower, upper)
+                    elsif alpha
+                        retval = alphacheck(value, ary)
+                    end
+
+                    if retval
+                        @parameters[type] = retval
+                    else
+                        raise Puppet::Error, "%s is not a valid %s" %
+                            [value, type]
+                    end
+                }
             end
 
             def paramminute=(value)
@@ -378,12 +514,15 @@ module Puppet
 
                 begin
                     obj = Etc.getpwnam(user)
+                    @uid = obj.uid
                 rescue ArgumentError
-                    raise Puppet::Error, "User %s not found"
+                    raise Puppet::Error, "User %s not found" % user
                 end
                 @parameters[:user] = user
             end
 
+            # Override the default Puppet::Type method because we need to call
+            # the +@crontype+ retrieve method.
             def retrieve
                 unless @parameters.include?(:user)
                     raise Puppet::Error, "You must specify the cron user"
@@ -393,10 +532,13 @@ module Puppet
                 @states[:command].retrieve
             end
 
+            # Write the entire user's cron tab out.
             def store
                 self.class.store(@parameters[:user])
             end
 
+            # Convert the current object a cron-style string.  Adds the cron name
+            # as a comment above the cron job, in the form '# Puppet Name: <name>'.
             def to_cron
                 hash = {:command => @states[:command].should || @states[:command].is }
                 self.class.fields().reject { |f| f == :command }.each { |param|
@@ -405,9 +547,15 @@ module Puppet
 
                 return "# Puppet Name: %s\n" % self.name +
                     self.class.fields.collect { |f|
-                        hash[f]
+                        if hash[f].is_a?(Array)
+                            hash[f].join(",")
+                        else
+                            hash[f]
+                        end
                     }.join(" ")
             end
         end
     end
 end
+
+# $Id$
diff --git a/lib/puppet/type/service.rb b/lib/puppet/type/service.rb
index a50af8ead..ff5fd779b 100644
--- a/lib/puppet/type/service.rb
+++ b/lib/puppet/type/service.rb
@@ -128,9 +128,10 @@ module Puppet
             @allowedmethods = [:setpath]
 
             def initialize(hash)
+                @searchpaths = []
                 super
 
-                unless defined? @searchpaths and @searchpaths.length >= 0
+                unless defined? @searchpaths and @searchpaths.length > 0
                     raise Puppet::Error.new(
                         "You must specify a valid search path for service %s" %
                         self.name
@@ -152,10 +153,11 @@ module Puppet
                     # if we've gotten this far, we found a valid script
                     return fqname
                 }
-                raise "Could not find init script for '%s'" % name
+                raise Puppet::Error, "Could not find init script for '%s'" % name
             end
 
             def parampath=(ary)
+                @parameters[:path] = ary
                 # verify each of the paths exists
                 @searchpaths = ary.find_all { |dir|
                     FileTest.directory?(dir)