On Jul 24, 2009, at 2:14 PM, 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
> separate private methods.
>
> All spec tests and unit tests pass. Some new spec tests
> are added as well.
>
> Signed-off-by: Steven Jenkins <[email protected]>
> ---
> lib/puppet/type/file.rb | 126
> +-------------------------------------------
> lib/puppet/util/backups.rb | 94 ++++++++++++++++++++++++++++++++
> spec/unit/util/backups.rb | 27 +++++++++
> 3 files changed, 123 insertions(+), 124 deletions(-)
> create mode 100644 lib/puppet/util/backups.rb
> create mode 100755 spec/unit/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
> -
> # 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..f76a52b
> --- /dev/null
> +++ b/lib/puppet/util/backups.rb
> @@ -0,0 +1,94 @@
> +module Puppet::Util::Backups
> +
> + # Deal with backups.
> + def handlebackup(file = nil)
> + # let the path be specified
> + file ||= self[:path]
> + return true unless FileTest.exists?(file)
> + # if they specifically don't want a backup, then just say
> + # we're good
> + return true unless self[:backup]
> + unless self.bucket || self[:backup]
> + self.err "Need a bucket or a backup setting for backups"
> + return false
> + end
This code isn't really necessary, is it? We already return if :backup
isn't set, which should cover all of the bases, since the 'bucket'
value can only be derived from :backup.
>
> + return handlebucket(file) if self.bucket
> + return handlebackuplocal(file, self[:backup])
Is it reasonable to get rid of the 'handle' term here? I think I did a
poor job of naming the method in the first place; I used to have a
tendency to use generic verbs all over the place -- evaluate, handle,
etc. It probably even makes sense to just change the 'handlebackup'
entry point to be just 'perform_backup' or something similar. And
while most old code doesn't do it, I've been working toward more
common naming methods: Longer method names, using underscores.
>
> + end
> +
> + private
> +
> + def handlebucket(fileobj)
> + file = (fileobj.class == String) ? fileobj : fileobj.name
> + case File.stat(file).ftype
> + when "directory"
> + # we don't need to backup directories when recurse is on
> + return true if self[:recurse]
> + info "Recursively backing up to filebucket"
> + require 'find'
> + Find.find(file) do |f|
> + if File.file?(f)
> + sum = self.bucket.backup(f)
> + self.info "Filebucketed %s to %s with sum %s" %
> + [f, self.bucket.name, sum]
> + end
> + end
> + return true
> + when "file"
> + sum = self.bucket.backup(file)
> + self.info "Filebucketed %s to %s with sum %s" %
> + [file, self.bucket.name, sum]
> + return true
> + when "link"; return true
> + end
> + end
I'd still like to see this method broken into two methods: One with
the case statement, and one to do the backing up.
>
> + def handlebackuplocal(fileobj, backup)
> + file = (fileobj.class == String) ? fileobj : fileobj.name
> + 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
> + end
> +
> + 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
> +end
> diff --git a/spec/unit/util/backups.rb b/spec/unit/util/backups.rb
> new file mode 100755
> index 0000000..2ec06e5
> --- /dev/null
> +++ b/spec/unit/util/backups.rb
> @@ -0,0 +1,27 @@
> +#!/usr/bin/env ruby
> +
> +require File.dirname(__FILE__) + '/../../spec_helper'
> +
> +require 'puppet/type/file'
> +require 'puppet/util/backups'
> +
> +describe Puppet::Util::Backups do
> + describe "when backing up a file" do
> + it "should succeed silently if the file does not exist" do
> + Puppet::Type::File.new(:name => "/no/such/
> file").handlebackup
> + end
> + it "should succeed silently if self[:backup] is false" do
> + Puppet::Type::File.new(:name => "/my/file", :backup =>
> false).handlebackup
> + end
> + it "should error if no bucket or backup suffix available" do
> + #lambda { Puppet::Type::File.new(:name => "/my/
> file", :backup => false, :bucket => false) }. should
> raise_error(Puppet::Error)
> + end
> +
> + # For the cases where there is an actual bucket or backup,
> + # cf spec/unit/type/file.rb section starting "when setting
> the backup"
> + # as those are the higher-level tests and cover the cases
> here.
> + # Also see test/ral/type/filebucket.rb and test/ral/type/
> file.rb,
> + # Testing #303, 304 are good examples
We should be moving all backup-related tests from the file tests into
this test module. Initially just copy them, make sure they all still
work, and once you're confident in the new tests, just add a test to
the file tests to verify this method gets called appropriately and
remove the old tests.
>
> + end
> +end
For the tests, you can make a simple test class that skips all of the
normal complexity:
class BackupTesting
include Puppet::Util::Backups
end
describe Puppet::Util::Backups do
before do
@backup = BackupTesting.new
end
...
end
Then you can skip all of the interactions with the type and focus on
the behaviour of this module. I'm even considering whether it makes
sense to pass the needed information to the method, so the file can
call it something like this:
perform_backup(self[:path], self[:backup], self.bucket)
I think that's overkill in this case, but it's appealing.
--
Morgan's Second Law:
To a first approximation all appointments are canceled.
---------------------------------------------------------------------
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
-~----------~----~----~----~------~----~------~--~---
