Fixing some naming problems with crons, and adding appropriate tests

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

2 files changed, 38 insertions, 16 deletions

diff --git a/lib/puppet/type/cron.rb b/lib/puppet/type/cron.rb
index fdaadb55b..ea34f4629 100755
--- a/lib/puppet/type/cron.rb
+++ b/lib/puppet/type/cron.rb
@@ -193,12 +193,21 @@ module Puppet
                 is used for human reference only and is generated automatically
                 for cron jobs found on the system.  This generally won't
                 matter, as Puppet will do its best to match existing cron jobs
-                against specified jobs (and Puppet adds a tag to cron jobs it
+                against specified jobs (and Puppet adds a comment to cron jobs it
                 adds), but it is at least possible that converting from
                 unmanaged jobs to managed jobs might require manual
-                intervention."
+                intervention.
+                
+                The names can only have alphanumeric characters plus the '-'
+                character."
 
             isnamevar
+
+            validate do |value|
+                unless value =~ /^[-\w]+$/
+                    raise ArgumentError, "Invalid name format '%s'" % value
+                end
+            end
         end
 
         newparam(:user) do
@@ -340,7 +349,7 @@ module Puppet
             end
             text.chomp.split("\n").each { |line|
                 case line
-                when /^# Puppet Name: (\w+)$/: name = $1
+                when /^# Puppet Name: (.+)$/: name = $1
                 when /^#/:
                     # add other comments to the list as they are
                     @instances[user] << line 
@@ -502,10 +511,6 @@ module Puppet
         def destroy
             # nothing, since the 'Cron.tab' method just doesn't write out
             # crons whose 'ensure' states are set to 'absent'.
-            #@states.each { |n, state|
-            #    next if n == :ensure
-            #    state.should == :absent
-            #}
             self.store
         end
 
diff --git a/test/types/cron.rb b/test/types/cron.rb
index d96d06e0b..496274d88 100755
--- a/test/types/cron.rb
+++ b/test/types/cron.rb
@@ -253,17 +253,18 @@ class TestCron < Test::Unit::TestCase
         tab = @fakefiletype.new(@me)
         tab.remove
 
-        name = "storeandretrieve"
-        cron = mkcron(name)
-        comp = newcomp(name, cron)
-        trans = assert_events([:cron_created], comp, name)
-        
-        cron = nil
+        %w{storeandretrieve a-name another-name more_naming SomeName}.each do |name|
+            cron = mkcron(name)
+            comp = newcomp(name, cron)
+            trans = assert_events([:cron_created], comp, name)
+            
+            cron = nil
 
-        Puppet.type(:cron).retrieve(@me)
+            Puppet.type(:cron).retrieve(@me)
 
-        assert(cron = Puppet.type(:cron)[name], "Could not retrieve named cron")
-        assert_instance_of(Puppet.type(:cron), cron)
+            assert(cron = Puppet.type(:cron)[name], "Could not retrieve named cron")
+            assert_instance_of(Puppet.type(:cron), cron)
+        end
     end
 
     # Do input validation testing on all of the parameters.
@@ -405,6 +406,22 @@ class TestCron < Test::Unit::TestCase
             @crontype.retrieve("nosuchuser")
         end
     end
+
+    def test_names
+        cron = mkcron("nametest")
+
+        ["bad name", "bad.name"].each do |name|
+            assert_raise(ArgumentError) do
+                cron[:name] = name
+            end
+        end
+
+        ["good-name", "good-name", "AGoodName"].each do |name|
+            assert_nothing_raised do
+                cron[:name] = name
+            end
+        end
+    end
 end
 
 # $Id$