Steven's submitting another form of this patch. On Jul 31, 2009, at 1:35 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 > separate private methods. > > Some new RSpec tests which use mock and stubs are added as well, > including removing the old test/ral/type/filebucket.rb tests > since they are already covered by RSpec tests. > > Signed-off-by: Steven Jenkins <[email protected]> > --- > lib/puppet/type/file.rb | 128 > +------------------------------------------ > lib/puppet/util/backups.rb | 86 +++++++++++++++++++++++++++++ > spec/unit/util/backups.rb | 68 +++++++++++++++++++++++ > test/ral/type/filebucket.rb | 110 > ------------------------------------- > 4 files changed, 157 insertions(+), 235 deletions(-) > create mode 100644 lib/puppet/util/backups.rb > create mode 100755 spec/unit/util/backups.rb > delete mode 100755 test/ral/type/filebucket.rb > > diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb > index 55d4ec7..ed5147e 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,39 +573,12 @@ 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) > return unless s = stat > > - self.fail "Could not back up; will not replace" unless > handlebackup > + self.fail "Could not back up; will not replace" unless > perform_backup > > unless should.to_s == "link" > return if s.ftype.to_s == should.to_s > diff --git a/lib/puppet/util/backups.rb b/lib/puppet/util/backups.rb > new file mode 100644 > index 0000000..b4bfbd2 > --- /dev/null > +++ b/lib/puppet/util/backups.rb > @@ -0,0 +1,86 @@ > +require 'find' > +module Puppet::Util::Backups > + > + # Deal with backups. > + def perform_backup(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] > + > + return perform_backup_with_bucket(file) if self.bucket > + return perform_backup_with_backuplocal(file, self[:backup]) > + end > + > + private > + > + def perform_backup_with_bucket(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" > + Find.find(self[:path]) { |f| > backup_file_with_filebucket(f) if > + File.file?(f) } > + when "file"; backup_file_with_filebucket(file) > + when "link"; return true > + end > + end > + > + def perform_backup_with_backuplocal(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 > + > + def backup_file_with_filebucket(f) > + sum = self.bucket.backup(f) > + self.info "Filebucketed %s to %s with sum %s" % [f, > self.bucket.name, sum] > + return sum > + end > +end > diff --git a/spec/unit/util/backups.rb b/spec/unit/util/backups.rb > new file mode 100755 > index 0000000..2ddab5e > --- /dev/null > +++ b/spec/unit/util/backups.rb > @@ -0,0 +1,68 @@ > +#!/usr/bin/env ruby > + > +require File.dirname(__FILE__) + '/../../spec_helper' > + > +require 'puppet/type/file' > +require 'puppet/util/backups' > +include PuppetTest > + > +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').perform_backup.should be_true > + end > + it "should succeed silently if self[:backup] is false" do > + FileTest.stubs(:exists?).returns true > + Puppet::Type::File.new(:name => '/some/file', :backup > => false).perform_backup.should be_true > + end > + it "a bucket should work when provided" do > + path = '/my/file' > + > + FileTest.stubs(:exists?).with(path).returns true > + > File.stubs(:stat).with(path).returns(mock('stat', :ftype => 'file')) > + > + bucket = mock('bucket', 'name' => 'foo') > + bucket.expects(:backup).with(path) > + > + file = Puppet::Type::File.new(:name => path, :backup => > 'foo') > + file.stubs(:bucket).returns bucket > + > + file.perform_backup.should be_nil > + end > + it "a local backup should work" do > + path = '/my/file' > + FileTest.stubs(:exists?).with(path).returns true > + > + file = Puppet::Type::File.new(:name => path, :backup => > '.foo') > + file.stubs(:perform_backup_with_backuplocal).returns true > + file.perform_backup.should be_true > + end > + end > + describe "when backing up a directory" do > + it "a bucket should work when provided" do > + path = '/my/dir' > + > + FileTest.stubs(:exists?).with(path).returns true > + > File.stubs(:stat).with(path).returns(mock('stat', :ftype => > 'directory')) > + Find.stubs(:find).returns('') > + > + #bucket = mock('bucket', 'name' => 'foo') > + bucket = mock('bucket') > + bucket.stubs(:backup).with(path).returns true > + > + file = Puppet::Type::File.new(:name => path, :backup => > 'foo') > + file.stubs(:bucket).returns bucket > + > + file.perform_backup.should be_true > + end > + it "a local backup should work" do > + path = '/my/dir' > + FileTest.stubs(:exists?).with(path).returns true > + > + file = Puppet::Type::File.new(:name => path, :backup => > '.foo') > + file.stubs(:perform_backup_with_backuplocal).returns true > + file.perform_backup.should be_true > + end > + end > +end > + > diff --git a/test/ral/type/filebucket.rb b/test/ral/type/filebucket.rb > deleted file mode 100755 > index b18a66f..0000000 > --- a/test/ral/type/filebucket.rb > +++ /dev/null > @@ -1,110 +0,0 @@ > -#!/usr/bin/env ruby > - > -require File.dirname(__FILE__) + '/../../lib/puppettest' > - > -require 'puppettest' > -require 'puppettest/support/utils' > -require 'fileutils' > - > -class TestFileBucket < Test::Unit::TestCase > - include PuppetTest::Support::Utils > - include PuppetTest::FileTesting > - # hmmm > - # this is complicated, because we store references to the created > - # objects in a central store > - def mkfile(hash) > - file = nil > - assert_nothing_raised { > - file = Puppet::Type.type(:file).new(hash) > - } > - return file > - end > - > - def mkbucket(name,path) > - bucket = nil > - assert_nothing_raised { > - bucket = Puppet::Type.type(:filebucket).new( > - :name => name, > - :path => path > - ) > - } > - > - @@tmpfiles.push path > - > - return bucket > - end > - > - def mktestfile > - # because luke's home directory is on nfs, it can't be used > for testing > - # as root > - tmpfile = tempfile() > - File.open(tmpfile, "w") { |f| f.puts rand(100) } > - @@tmpfiles.push tmpfile > - mkfile(:name => tmpfile) > - end > - > - def setup > - super > - begin > - initstorage > - rescue > - system("rm -rf %s" % Puppet[:statefile]) > - end > - end > - > - def initstorage > - Puppet::Util::Storage.init > - Puppet::Util::Storage.load > - end > - > - def clearstorage > - Puppet::Util::Storage.store > - Puppet::Util::Storage.clear > - end > - > - def test_simplebucket > - name = "yayness" > - bucketpath = tempfile() > - resource = mkbucket(name, bucketpath) > - > - bucket = resource.bucket > - > - assert_instance_of(Puppet::Network::Client.dipper, bucket) > - > - md5 = nil > - newpath = tempfile() > - @@tmpfiles << newpath > - system("cp /etc/passwd %s" % newpath) > - assert_nothing_raised { > - md5 = bucket.backup(newpath) > - } > - > - assert(md5) > - > - dir, file, pathfile = > Puppet::Network::Handler.filebucket.paths(bucketpath, md5) > - > - assert(FileTest.directory?(dir), > - "MD5 directory does not exist") > - > - newmd5 = nil > - > - # Just in case the file isn't writable > - File.chmod(0644, newpath) > - File.open(newpath, "w") { |f| f.puts > ";lkjasdf;lkjasdflkjwerlkj134lkj" } > - > - assert_nothing_raised { > - newmd5 = bucket.backup(newpath) > - } > - > - assert(md5 != newmd5) > - > - assert_nothing_raised { > - bucket.restore(newpath, md5) > - } > - > - File.open(newpath) { |f| newmd5 = > Digest::MD5.hexdigest(f.read) } > - > - assert_equal(md5, newmd5) > - end > -end > - > -- > 1.6.3.3 > > > > -- The brain is a wonderful organ. It starts working the moment you get up in the morning and does not stop until you get into the office. --Robert Frost --------------------------------------------------------------------- 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 -~----------~----~----~----~------~----~------~--~---
