On Jul 24, 2009, at 9:43 AM, Steven Jenkins wrote:
>
> This changes the condition checking of handlebucket, as well as
> moves it (and remove_backup) into a separate module. It
> additionally refactors common code out of handlebucket into
> a separate (now private) method.
>
> All spec tests still pass; however, test/ral/type/file.rb:696 fails.
>
> Signed-off-by: Steven Jenkins <[email protected]>
> ---
> lib/puppet/type/file.rb | 126
> +-------------------------------------------
> lib/puppet/util/backups.rb | 110 +++++++++++++++++++++++++++++++++++
> +++
> 2 files changed, 112 insertions(+), 124 deletions(-)
> create mode 100644 lib/puppet/util/backups.rb
>
> diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
> index 55d4ec7..657c7ab 100644
> --- a/lib/puppet/type/file.rb
> +++ b/lib/puppet/type/file.rb
> @@ -7,11 +7,13 @@ require 'puppet/network/handler'
> require 'puppet/util/diff'
> require 'puppet/util/checksums'
> require 'puppet/network/client'
> +require 'puppet/util/backups'
>
> module Puppet
> newtype(:file) do
> include Puppet::Util::MethodHelper
> include Puppet::Util::Checksums
> + include Puppet::Util::Backups
> @doc = "Manages local files, including setting ownership and
> permissions, creation of both files and directories, and
> retrieving entire files from remote servers. As Puppet
> matures, it
> @@ -391,103 +393,6 @@ module Puppet
> @stat = nil
> end
>
> - # Deal with backups.
> - def handlebackup(file = nil)
> - # let the path be specified
> - file ||= self[:path]
> - # if they specifically don't want a backup, then just say
> - # we're good
> - unless FileTest.exists?(file)
> - return true
> - end
> -
> - unless self[:backup]
> - return true
> - end
> -
> - case File.stat(file).ftype
> - when "directory"
> - if self[:recurse]
> - # we don't need to backup directories when
> recurse is on
> - return true
> - else
> - backup = self.bucket || self[:backup]
> - case backup
> - when Puppet::Network::Client.client(:Dipper)
> - notice "Recursively backing up to filebucket"
> - require 'find'
> - Find.find(self[:path]) do |f|
> - if File.file?(f)
> - sum = backup.backup(f)
> - self.notice "Filebucketed %s to %s
> with sum %s" %
> - [f, backup.name, sum]
> - end
> - end
> -
> - return true
> - when String
> - newfile = file + backup
> - # Just move it, since it's a directory.
> - if FileTest.exists?(newfile)
> - remove_backup(newfile)
> - end
> - begin
> - bfile = file + backup
> -
> - # Ruby 1.8.1 requires the 'preserve'
> addition, but
> - # later versions do not appear to
> require it.
> - FileUtils.cp_r(file, bfile, :preserve
> => true)
> - return true
> - rescue => detail
> - # since they said they want a backup,
> let's error out
> - # if we couldn't make one
> - self.fail "Could not back %s up: %s" %
> - [file, detail.message]
> - end
> - else
> - self.err "Invalid backup type %s" %
> backup.inspect
> - return false
> - end
> - end
> - when "file"
> - backup = self.bucket || self[:backup]
> - case backup
> - when Puppet::Network::Client.client(:Dipper)
> - sum = backup.backup(file)
> - self.notice "Filebucketed to %s with sum %s" %
> - [backup.name, sum]
> - return true
> - when String
> - newfile = file + backup
> - if FileTest.exists?(newfile)
> - remove_backup(newfile)
> - end
> - begin
> - # FIXME Shouldn't this just use a Puppet
> object with
> - # 'source' specified?
> - bfile = file + backup
> -
> - # Ruby 1.8.1 requires the 'preserve'
> addition, but
> - # later versions do not appear to require it.
> - FileUtils.cp(file, bfile, :preserve => true)
> - return true
> - rescue => detail
> - # since they said they want a backup, let's
> error out
> - # if we couldn't make one
> - self.fail "Could not back %s up: %s" %
> - [file, detail.message]
> - end
> - else
> - self.err "Invalid backup type %s" %
> backup.inspect
> - return false
> - end
> - when "link"; return true
> - else
> - self.notice "Cannot backup files of type %s" %
> File.stat(file).ftype
> - return false
> - end
> - end
> -
> def initialize(hash)
> # Used for caching clients
> @clients = {}
> @@ -668,33 +573,6 @@ module Puppet
> Puppet::FileServing::Metadata.search(path, :links =>
> self[:links], :recurse => (self[:recurse] == :remote ? true :
> self[:recurse]), :recurselimit => self[:recurselimit], :ignore =>
> self[:ignore])
> end
>
> - # Remove the old backup.
> - def remove_backup(newfile)
> - if self.class.name == :file and self[:links] != :follow
> - method = :lstat
> - else
> - method = :stat
> - end
> - old = File.send(method, newfile).ftype
> -
> - if old == "directory"
> - raise Puppet::Error,
> - "Will not remove directory backup %s; use a
> filebucket" %
> - newfile
> - end
> -
> - info "Removing old backup of type %s" %
> - File.send(method, newfile).ftype
> -
> - begin
> - File.unlink(newfile)
> - rescue => detail
> - puts detail.backtrace if Puppet[:trace]
> - self.err "Could not remove old backup: %s" % detail
> - return false
> - end
> - end
> -
It's great to see all of that code removed.
>
> # Remove any existing data. This is only used when dealing
> with
> # links or directories.
> def remove_existing(should)
> diff --git a/lib/puppet/util/backups.rb b/lib/puppet/util/backups.rb
> new file mode 100644
> index 0000000..89b0505
> --- /dev/null
> +++ b/lib/puppet/util/backups.rb
> @@ -0,0 +1,110 @@
> +module Puppet::Util::Backups
> +
> + # Deal with backups.
> + def handlebackup(file = nil)
> + # let the path be specified
> + file ||= self[:path]
> + # if they specifically don't want a backup, then just say
> + # we're good
> + unless FileTest.exists?(file)
> + return true
> + end
> +
> + unless self[:backup]
> + return true
> + end
Both of these can be trimmed up a bit using this idiom:
return true unless FileTest.exists?(file)
>
> + backup = self.bucket || self[:backup]
This line should be removed; I'm pretty sure this value isn't used
anywhere.
>
> + case File.stat(file).ftype
> + when "directory"
> + # we don't need to backup directories when recurse
> is on
> + return true if self[:recurse]
> +
> + if self.bucket
> + notice "Recursively backing up to filebucket"
> + require 'find'
> + Find.find(self[:path]) do |f|
> + if File.file?(f)
> + sum = backup.backup(f)
> + self.notice "Filebucketed %s to %s with
> sum %s" %
> + [f, backup.name, sum]
Can you change all of the notice logs here to info? They really
shouldn't be at the normal log level; I've had a customer complain
about that. It's an unrelated problem, but this method seems to be
the main culprit.
>
> + end
> + end
> +
> + return true
> + elsif self[:backup]
> + handlebackuplocal(file)
> + else
> + self.err "Invalid backup type %s" %
> backup.inspect
> + return false
This branch can be removed because we're doing the validation in the
parameter - we know we'll only ever have valid values here.
>
> + end
> + when "file"
> + if self.bucket
> + sum = backup.backup(file)
> + self.notice "Filebucketed to %s with sum %s" %
> + [backup.name, sum]
> + return true
> + elsif self[:backup]
> + handlebackuplocal(file)
> + else
> + self.err "Invalid backup type %s" %
> backup.inspect
> + return false
This validation is also unnecessary.
>
> + end
> + when "link"; return true
> + else
> + self.notice "Cannot backup files of type %s" %
> File.stat(file).ftype
> + return false
> + end
> + end
I think this whole method would be better off refactored into a
dispatch-based structure instead of the case statements. Something
like:
return backup_with_filebucket if self.bucket
return backup_with_file
Each of these would then handle the file/dir differences. Well,
really, you'd need something like:
def backup_with_filebucket
ftype = stat().ftype # this is part of the file type already
case ftype
when "directory"
Find.find(self[:path]) { |f| backup_file_with_filebucket(f) if
File.file?(f) }
when "file"; backup_file_with_filebucket(f)
when "link"; return true
end
end
This should allow all of the methods to be very short and easily
testable.
Speaking of tests, there, um, don't appear to be any. :)
> +private
> +
> + def remove_backup(newfile)
> + if self.class.name == :file and self[:links] != :follow
> + method = :lstat
> + else
> + method = :stat
> + end
> + old = File.send(method, newfile).ftype
> +
> + if old == "directory"
> + raise Puppet::Error,
> + "Will not remove directory backup %s; use a
> filebucket" %
> + newfile
> + end
> +
> + info "Removing old backup of type %s" %
> + File.send(method, newfile).ftype
> +
> + begin
> + File.unlink(newfile)
> + rescue => detail
> + puts detail.backtrace if Puppet[:trace]
> + self.err "Could not remove old backup: %s" % detail
> + return false
> + end
> + end
> +
> + def handlelocalbackup(file)
> + newfile = file + backup
> + if FileTest.exists?(newfile)
> + remove_backup(newfile)
> + end
> +
> + begin
> + bfile = file + backup
> +
> + # Ruby 1.8.1 requires the 'preserve' addition, but
> + # later versions do not appear to require it.
> + # N.B. cp_r works on both files and directories
> + FileUtils.cp_r(file, bfile, :preserve => true)
> + return true
> + rescue => detail
> + # since they said they want a backup, let's error out
> + # if we couldn't make one
> + self.fail "Could not back %s up: %s" %
> + [file, detail.message]
> + end
There seem to be some weird indentation issues here -- are you using 4
spaces throughout?
>
> + end
> +
> +end
> --
> 1.6.3.3
>
>
> >
--
A cult is a religion with no political power. -- Tom Wolfe
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
