Refactoring part of the file/filebucket integration

The goal of this commit is to fix ordering issues
that could result when the filebuckets are added
to the catalog after the resources that use them.

This condition showed up somewhat arbitrarily.

Signed-off-by: Luke Kanies <luke@madstop.com>

5 files changed, 93 insertions, 110 deletions

diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 721b5a1fa..55d4ec73b 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -87,7 +87,7 @@ module Puppet
                 filebucketed files.
                 "
 
-            defaultto { "puppet" }
+            defaultto "puppet"
 
             munge do |value|
                 # I don't really know how this is happening.
@@ -98,27 +98,10 @@ module Puppet
                     false
                 when true, "true", ".puppet-bak", :true
                     ".puppet-bak"
-                when /^\./
-                    value
                 when String
-                    # We can't depend on looking this up right now,
-                    # we have to do it after all of the objects
-                    # have been instantiated.
-                    if resource.catalog and bucketobj = resource.catalog.resource(:filebucket, value)
-                        @resource.bucket = bucketobj.bucket
-                        bucketobj.title
-                    else
-                        # Set it to the string; finish() turns it into a
-                        # filebucket.
-                        @resource.bucket = value
-                        value
-                    end
-                when Puppet::Network::Client.client(:Dipper)
-                    @resource.bucket = value
-                    value.name
+                    value
                 else
-                    self.fail "Invalid backup type %s" %
-                        value.inspect
+                    self.fail "Invalid backup type %s" % value.inspect
                 end
             end
         end
@@ -250,8 +233,6 @@ module Puppet
             newvalues(:first, :all)
         end
 
-        attr_accessor :bucket
-
         # Autorequire any parent directories.
         autorequire(:file) do
             if self[:path]
@@ -349,6 +330,32 @@ module Puppet
             return asuser
         end
 
+        def bucket
+            return @bucket if defined?(@bucket) and @bucket
+
+            backup = self[:backup]
+            return nil unless backup
+            return nil if backup =~ /^\./
+
+            unless catalog or backup == "puppet"
+                fail "Can not find filebucket for backups without a catalog"
+            end
+
+            unless catalog and filebucket = catalog.resource(:filebucket, backup) or backup == "puppet"
+                fail "Could not find filebucket %s specified in backup" % backup
+            end
+
+            return default_bucket unless filebucket
+
+            @bucket = filebucket.bucket
+
+            return @bucket
+        end
+
+        def default_bucket
+            Puppet::Type.type(:filebucket).mkdefaultbucket.bucket
+        end
+
         # Does the file currently exist?  Just checks for whether
         # we have a stat
         def exist?
@@ -359,26 +366,7 @@ module Puppet
         # there is one.
         def finish
             # Look up our bucket, if there is one
-            if bucket = self.bucket
-                case bucket
-                when String
-                    if catalog and obj = catalog.resource(:filebucket, bucket)
-                        self.bucket = obj.bucket
-                    elsif bucket == "puppet"
-                        obj = Puppet::Network::Client.client(:Dipper).new(
-                            :Path => Puppet[:clientbucketdir]
-                        )
-                        self.bucket = obj
-                    else
-                        self.fail "Could not find filebucket '%s'" % bucket
-                    end
-                when Puppet::Network::Client.client(:Dipper) # things are hunky-dorey
-                when Puppet::Type::Filebucket # things are hunky-dorey
-                    self.bucket = bucket.bucket
-                else
-                    self.fail "Invalid bucket type %s" % bucket.class
-                end
-            end
+            bucket()
             super
         end
 
diff --git a/spec/integration/transaction.rb b/spec/integration/transaction.rb
index 17aba0df8..c06a43d80 100755
--- a/spec/integration/transaction.rb
+++ b/spec/integration/transaction.rb
@@ -7,10 +7,10 @@ require 'puppet/transaction'
 describe Puppet::Transaction do
     it "should not apply generated resources if the parent resource fails" do
         catalog = Puppet::Resource::Catalog.new
-        resource = Puppet::Type.type(:file).new :path => "/foo/bar"
+        resource = Puppet::Type.type(:file).new :path => "/foo/bar", :backup => false
         catalog.add_resource resource
 
-        child_resource = Puppet::Type.type(:file).new :path => "/foo/bar/baz"
+        child_resource = Puppet::Type.type(:file).new :path => "/foo/bar/baz", :backup => false
 
         resource.expects(:eval_generate).returns([child_resource])
 
diff --git a/spec/integration/type/file.rb b/spec/integration/type/file.rb
index 2877153f0..cced1ed1c 100755
--- a/spec/integration/type/file.rb
+++ b/spec/integration/type/file.rb
@@ -33,7 +33,7 @@ describe Puppet::Type.type(:file) do
         it "should be able to recurse over a nonexistent file" do
             @path = tmpfile("file_integration_tests")
 
-            @file = Puppet::Type::File.new(:name => @path, :mode => 0644, :recurse => true)
+            @file = Puppet::Type::File.new(:name => @path, :mode => 0644, :recurse => true, :backup => false)
 
             @catalog = Puppet::Resource::Catalog.new
             @catalog.add_resource @file
@@ -46,7 +46,7 @@ describe Puppet::Type.type(:file) do
 
             build_path(@path)
 
-            @file = Puppet::Type::File.new(:name => @path, :mode => 0644, :recurse => true)
+            @file = Puppet::Type::File.new(:name => @path, :mode => 0644, :recurse => true, :backup => false)
 
             @catalog = Puppet::Resource::Catalog.new
             @catalog.add_resource @file
@@ -69,7 +69,7 @@ describe Puppet::Type.type(:file) do
 
             dest = tmpfile("file_link_integration_dest")
 
-            @file = Puppet::Type::File.new(:name => dest, :target => source, :recurse => true, :ensure => :link)
+            @file = Puppet::Type::File.new(:name => dest, :target => source, :recurse => true, :ensure => :link, :backup => false)
 
             @catalog = Puppet::Resource::Catalog.new
             @catalog.add_resource @file
@@ -96,7 +96,7 @@ describe Puppet::Type.type(:file) do
 
             dest = tmpfile("file_source_integration_dest")
 
-            @file = Puppet::Type::File.new(:name => dest, :source => source, :recurse => true)
+            @file = Puppet::Type::File.new(:name => dest, :source => source, :recurse => true, :backup => false)
 
             @catalog = Puppet::Resource::Catalog.new
             @catalog.add_resource @file
@@ -131,7 +131,7 @@ describe Puppet::Type.type(:file) do
             File.open(s1, "w") { |f| f.puts "uno" }
             File.open(s2, "w") { |f| f.puts "dos" }
 
-            @file = Puppet::Type::File.new(:name => @dest, :source => @source, :recurse => true)
+            @file = Puppet::Type::File.new(:name => @dest, :source => @source, :recurse => true, :backup => false)
 
             @catalog = Puppet::Resource::Catalog.new
             @catalog.add_resource @file
@@ -197,7 +197,7 @@ describe Puppet::Type.type(:file) do
 
             File.open(source, "w") { |f| f.print "foo" }
 
-            file = Puppet::Type::File.new(:name => dest, :source => source)
+            file = Puppet::Type::File.new(:name => dest, :source => source, :backup => false)
 
             catalog = Puppet::Resource::Catalog.new
             catalog.add_resource file
@@ -269,7 +269,8 @@ describe Puppet::Type.type(:file) do
         file = Puppet::Type.type(:file).new(
             :name => dest,
             :ensure => :absent,
-            :source => source
+            :source => source,
+            :backup => false
         )
 
         catalog = Puppet::Resource::Catalog.new
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index cdc4ffd57..58cd4ad23 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -680,7 +680,7 @@ describe Puppet::Type.type(:file) do
             Puppet::Type::File.new(:name => "/my/file", :backup => ".bak")[:backup].should == ".bak"
         end
 
-        it "should set the file bucket when backup is set to a string matching the name of a filebucket in the catalog" do
+        it "should set the filebucket when backup is set to a string matching the name of a filebucket in the catalog" do
             catalog = Puppet::Resource::Catalog.new
             bucket_resource = Puppet::Type.type(:filebucket).new :name => "foo", :path => "/my/file/bucket"
             catalog.add_resource bucket_resource
@@ -703,5 +703,52 @@ describe Puppet::Type.type(:file) do
 
             file.bucket.should == bucket_resource.bucket
         end
+
+        it "should have a nil filebucket if backup is false" do
+            catalog = Puppet::Resource::Catalog.new
+            bucket_resource = Puppet::Type.type(:filebucket).new :name => "foo", :path => "/my/file/bucket"
+            catalog.add_resource bucket_resource
+
+            file = Puppet::Type::File.new(:name => "/my/file", :backup => false)
+            catalog.add_resource file
+
+            file.bucket.should be_nil
+        end
+
+        it "should have a nil filebucket if backup is set to a string starting with '.'" do
+            catalog = Puppet::Resource::Catalog.new
+            bucket_resource = Puppet::Type.type(:filebucket).new :name => "foo", :path => "/my/file/bucket"
+            catalog.add_resource bucket_resource
+
+            file = Puppet::Type::File.new(:name => "/my/file", :backup => ".foo")
+            catalog.add_resource file
+
+            file.bucket.should be_nil
+        end
+
+        it "should fail if there's no catalog and backup is not false" do
+            file = Puppet::Type::File.new(:name => "/my/file", :backup => "foo")
+
+            lambda { file.bucket }.should raise_error(Puppet::Error)
+        end
+
+        it "should fail if a non-existent catalog is specified" do
+            file = Puppet::Type::File.new(:name => "/my/file", :backup => "foo")
+            catalog = Puppet::Resource::Catalog.new
+            catalog.add_resource file
+
+            lambda { file.bucket }.should raise_error(Puppet::Error)
+        end
+
+        it "should be able to use the default filebucket without a catalog" do
+            file = Puppet::Type::File.new(:name => "/my/file", :backup => "puppet")
+            file.bucket.should be_instance_of(Puppet::Network::Client::Dipper)
+        end
+
+        it "should look up the filebucket during finish()" do
+            file = Puppet::Type::File.new(:name => "/my/file", :backup => ".foo")
+            file.expects(:bucket)
+            file.finish
+        end
     end
 end
diff --git a/test/ral/type/file.rb b/test/ral/type/file.rb
index 8db306c32..829310ed4 100755
--- a/test/ral/type/file.rb
+++ b/test/ral/type/file.rb
@@ -577,7 +577,8 @@ class TestFile < Test::Unit::TestCase
             file = Puppet::Type.type(:file).new(
                 :name => dest,
                 :checksum => "md5",
-                :content => "This is some content"
+                :content => "This is some content",
+                :backup => false
             )
         }
 
@@ -726,7 +727,8 @@ class TestFile < Test::Unit::TestCase
         assert_nothing_raised {
             file = Puppet::Type.type(:file).new(
                 :ensure => path,
-                :path => link
+                :path => link,
+                :backup => false
             )
         }
 
@@ -981,51 +983,6 @@ class TestFile < Test::Unit::TestCase
         assert_equal(:false, file[:replace], ":replace did not alias :false to :no")
     end
 
-    def test_backup
-        path = tempfile()
-        file = Puppet::Type.newfile :path => path, :content => "yay"
-
-        catalog = mk_catalog(file)
-        catalog.finalize # adds the default resources.
-
-        [false, :false, "false"].each do |val|
-            assert_nothing_raised do
-                file[:backup] = val
-            end
-            assert_equal(false, file[:backup], "%s did not translate" % val.inspect)
-        end
-        [true, :true, "true", ".puppet-bak"].each do |val|
-            assert_nothing_raised do
-                file[:backup] = val
-            end
-            assert_equal(".puppet-bak", file[:backup], "%s did not translate" % val.inspect)
-        end
-
-        # Now try a non-bucket string
-        assert_nothing_raised do
-            file[:backup] = ".bak"
-        end
-        assert_equal(".bak", file[:backup], ".bak did not translate")
-
-        # Now try a non-existent bucket
-        assert_nothing_raised do
-            file[:backup] = "main"
-        end
-        assert_equal("main", file[:backup], "bucket name was not retained")
-        assert_equal("main", file.bucket, "file's bucket was not set")
-
-        # And then an existing bucket
-        obj = Puppet::Type.type(:filebucket).new :name => "testing"
-        catalog.add_resource(obj)
-        bucket = obj.bucket
-
-        assert_nothing_raised do
-            file[:backup] = "testing"
-        end
-        assert_equal("testing", file[:backup], "backup value was reset")
-        assert_equal(obj.bucket, file.bucket, "file's bucket was not set")
-    end
-
     def test_pathbuilder
         dir = tempfile()
         Dir.mkdir(dir)
@@ -1047,7 +1004,7 @@ class TestFile < Test::Unit::TestCase
     def test_removal_with_content_set
         path = tempfile()
         File.open(path, "w") { |f| f.puts "yay" }
-        file = Puppet::Type.newfile(:name => path, :ensure => :absent, :content => "foo")
+        file = Puppet::Type.newfile(:name => path, :ensure => :absent, :content => "foo", :backup => false)
 
         assert_apply(file)
         assert(! FileTest.exists?(path), "File was not removed")
@@ -1125,16 +1082,6 @@ class TestFile < Test::Unit::TestCase
     end
     end
 
-    # Make sure we default to the "puppet" filebucket, rather than a string
-    def test_backup_defaults_to_bucket
-        path = tempfile
-        file = Puppet::Type.newfile(:path => path, :content => 'some content')
-        file.finish
-
-        assert_instance_of(Puppet::Network::Client::Dipper, file.bucket,
-            "did not default to a filebucket for backups")
-    end
-
     # #567
     def test_missing_files_are_in_sync
         file = tempfile