[PATCH/puppet 1/1] Fixed #2798 - Fixes cron type for AIX

[James Turnbull]

Patch provided by Andrew Forgue

Signed-off-by: James Turnbull <ja...@lovedthanlost.net>
---
lib/puppet/provider/cron/crontab.rb | 2 +
lib/puppet/util/filetype.rb | 45 +++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/lib/puppet/provider/cron/crontab.rb b/lib/puppet/provider/cron/crontab.rb
index 82384d0..6dee2e5 100755
--- a/lib/puppet/provider/cron/crontab.rb
+++ b/lib/puppet/provider/cron/crontab.rb
@@ -3,6 +3,8 @@ require 'puppet/provider/parsedfile'
tab = case Facter.value(:operatingsystem)
when "Solaris"
:suntab
+ when "AIX"
+ :aixtab
else
:crontab
end
diff --git a/lib/puppet/util/filetype.rb b/lib/puppet/util/filetype.rb
index 93c002f..b66a1a1 100755
--- a/lib/puppet/util/filetype.rb
+++ b/lib/puppet/util/filetype.rb
@@ -251,4 +251,49 @@ class Puppet::Util::FileType
output_file.delete
end
end
+
+ # Support for AIX crontab with differing ouput from suntab jon crontab command.
+ newfiletype(:aixtab) do
+ # Read a specific @path's cron tab.
+ def read
+ begin
+ output = Puppet::Util.execute(%w{crontab -l}, :uid => @path)
+ return "" if output.include?("You are not authorized to use the cron command")
+ raise Puppet::Error, "User %s not authorized to use cron" % @path if output.include?("You are not authorized to use the cron command")
+ return output
+ rescue => detail
+ raise Puppet::Error, "Could not read crontab for %s: %s" % [@path, detail]
+ end
+ end
+
+ # Remove a specific @path's cron tab.
+ def remove
+ begin
+ Puppet::Util.execute(%w{crontab -r}, :uid => @path)
+ rescue => detail
+ raise Puppet::Error, "Could not remove crontab for %s: %s" % [@path, detail]
+ end
+ end
+
+ # Overwrite a specific @path's cron tab; must be passed the @path name
+ # and the text with which to create the cron tab.
+ def write(text)
+ puts text
+ require "tempfile"
+ output_file = Tempfile.new("puppet")
+ fh = output_file.open
+ fh.print text
+ fh.close
+
+ # We have to chown the stupid file to the user.
+ File.chown(Puppet::Util.uid(@path), nil, output_file.path)
+
+ begin
+ Puppet::Util.execute(["crontab", output_file.path], :uid => @path)
+ rescue => detail
+ raise Puppet::Error, "Could not write crontab for %s: %s" % [@path, detail]
+ end
+ output_file.delete
+ end
+ end
end
--
1.6.2.5

[Markus Roberts]

+    #  Support for AIX crontab with differing ouput from suntab jon crontab command.

+    newfiletype(:aixtab) do
+        # Read a specific @path's cron tab.
+        def read
+            begin
+                output = Puppet::Util.execute(%w{crontab -l}, :uid => @path)
+                return "" if output.include?("You are not authorized to use the cron command")
+                raise Puppet::Error, "User %s not authorized to use cron" % @path if output.include?("You are not authorized to use the cron command")

This raise will never get called, since 1) the strings coming back from execute are "\n" terminated and 2) even if they weren't, the return on the line above would have cause an early exit.  Also, the return is passing back a string whereas the normal return type appears to be an array of strings:
 

+                return output
+            rescue => detail
+                raise Puppet::Error, "Could not read crontab for %s: %s" % [@path, detail]
+            end
+        end
+
+        # Remove a specific @path's cron tab.
+        def remove
+            begin
+                Puppet::Util.execute(%w{crontab -r}, :uid => @path)
+            rescue => detail
+                raise Puppet::Error, "Could not remove crontab for %s: %s" % [@path, detail]
+            end
+        end
+
+        # Overwrite a specific @path's cron tab; must be passed the @path name
+        # and the text with which to create the cron tab.
+        def write(text)
+            puts text

Do we really want to be echoing the text to STDOUT all the time?
 

+            require "tempfile"
+            output_file = Tempfile.new("puppet")
+            fh = output_file.open
+            fh.print text
+            fh.close

+            # We have to chown the stupid file to the user.
+            File.chown(Puppet::Util.uid(@path), nil, output_file.path)
+
+            begin
+                Puppet::Util.execute(["crontab", output_file.path], :uid => @path)
+            rescue => detail
+                raise Puppet::Error, "Could not write crontab for %s: %s" % [@path, detail]
+            end
+            output_file.delete

We could say that we don't really need to delete the output file (since it was created with tempfile and will be cleaned up on exit) but if we do decide that we're going to clean it up (because we're a long-running process) we should do so in an ensure block, not bare and dangling.

[Andrew Forgue]

On Nov 24, 5:45 pm, Markus Roberts <mar...@reductivelabs.com> wrote:
> +    #  Support for AIX crontab with differing ouput from suntab jon crontab
> command.
>
> > +    newfiletype(:aixtab) do
> > +        # Read a specific @path's cron tab.
> > +        def read
> > +            begin
> > +                output = Puppet::Util.execute(%w{crontab -l}, :uid =>
> > @path)
> > +                return "" if output.include?("You are not authorized to
> > use the cron command")
> > +                raise Puppet::Error, "User %s not authorized to use cron"
> > % @path if output.include?("You are not authorized to use the cron command")
>
> This raise will never get called, since 1) the strings coming back from
> execute are "\n" terminated and 2) even if they weren't, the return on the
> line above would have cause an early exit.  Also, the return is passing back
> a string whereas the normal return type appears to be an array of strings:

I updated for #2 by removing the return "" if ... since AIX crontab
doesn't reply with anything other than 'You are not authorized'. I
don't understand what you mean by strings coming back from execute are
\n terminated; It looks like output is a big string with newlines
embedded, so include? should still work. The normal return from the
other cron types is a single string with embedded newlines as well. I
may be reading it wrong though.

> > +                return output
> > +            rescue => detail
> > +                raise Puppet::Error, "Could not read crontab for %s: %s" %
> > [@path, detail]
> > +            end
> > +        end
> > +
> > +        # Remove a specific @path's cron tab.
> > +        def remove
> > +            begin
> > +                Puppet::Util.execute(%w{crontab -r}, :uid => @path)
> > +            rescue => detail
> > +                raise Puppet::Error, "Could not remove crontab for %s: %s"
> > % [@path, detail]
> > +            end
> > +        end
> > +
> > +        # Overwrite a specific @path's cron tab; must be passed the @path
> > name
> > +        # and the text with which to create the cron tab.
> > +        def write(text)
> > +            puts text
>
> Do we really want to be echoing the text to STDOUT all the time?
>

Nope, fixed. I tihnk a bug needs to be filed to fix the suntab type
since that does it as well.

> > +            require "tempfile"
> > +            output_file = Tempfile.new("puppet")
> > +            fh = output_file.open
> > +            fh.print text
> > +            fh.close
>
> > +            # We have to chown the stupid file to the user.
> > +            File.chown(Puppet::Util.uid(@path), nil, output_file.path)
> > +
> > +            begin
> > +                Puppet::Util.execute(["crontab", output_file.path], :uid
> > => @path)
> > +            rescue => detail
> > +                raise Puppet::Error, "Could not write crontab for %s: %s"
> > % [@path, detail]
> > +            end
> > +            output_file.delete
>
> We could say that we don't really need to delete the output file (since it
> was created with tempfile and will be cleaned up on exit) but if we do
> decide that we're going to clean it up (because we're a long-running
> process) we should do so in an ensure block, not bare and dangling.

Done...

commit @
http://github.com/ajf/puppet/commit/95a78d75d13914b71156c38dbefb664c4a27eb99

[Markus Roberts]

+1

> I updated for #2 by removing the return "" if ... since AIX crontab
> doesn't reply with anything other than 'You are not authorized'.  I
> don't understand what you mean by strings coming back from execute are

> \n terminated;...

I was misreading your intent with the return "" line. The present
code makes sense and should work fine.

-- Markus