Whoops, forgot to update some spec tests to adapt to the new change. One of
the spec tests needed to be changed; the others were redundant (and
excessively stubbed) so we deleted them.
Amended the following patch:
diff --git a/spec/unit/file_bucket/dipper_spec.rb
b/spec/unit/file_bucket/dipper_spec.rb
index db40c62..c40d795 100755
--- a/spec/unit/file_bucket/dipper_spec.rb
+++ b/spec/unit/file_bucket/dipper_spec.rb
@@ -92,7 +92,7 @@ describe Puppet::FileBucket::Dipper do
[request1, request2].each do |r|
r.server.should == 'puppetmaster'
r.port.should == 31337
- r.key.should == "md5/#{checksum}"
+ r.key.should == "md5/#{checksum}#{real_path}"
end
end
diff --git a/spec/unit/file_bucket/file_spec.rb
b/spec/unit/file_bucket/file_spec.rb
index 82063c2..f80b162 100644
--- a/spec/unit/file_bucket/file_spec.rb
+++ b/spec/unit/file_bucket/file_spec.rb
@@ -64,30 +64,6 @@ describe Puppet::FileBucket::File do
end
end
- describe "when saving files" do
- it "should save the contents to the calculated path" do
- ::File.stubs(:directory?).with(@dir).returns(true)
- ::File.expects(:exist?).with("#{@dir}/contents").returns false
-
- mockfile = mock "file"
- mockfile.expects(:print).with(@contents)
- ::File.expects(:open).with("#{@dir}/contents",
::File::WRONLY|::File::CREAT, 0440).yields(mockfile)
-
- Puppet::FileBucket::File.new(@contents).save
- end
-
- it "should make any directories necessary for storage" do
- FileUtils.expects(:mkdir_p).with do |arg|
- ::File.umask == 0007 and arg == @dir
- end
- ::File.expects(:directory?).with(@dir).returns(false)
- ::File.expects(:open).with("#{@dir}/contents",
::File::WRONLY|::File::CREAT, 0440)
- ::File.expects(:exist?).with("#{@dir}/contents").returns false
-
- Puppet::FileBucket::File.new(@contents).save
- end
- end
-
it "should return a url-ish name" do
Puppet::FileBucket::File.new(@contents).name.should ==
"md5/4a8ec4fa5f01b4ab1a0ab8cbccb709f0"
end
@@ -105,17 +81,6 @@ describe Puppet::FileBucket::File do
Puppet::FileBucket::File.from_pson({"contents"=>"file
contents"}).contents.should == "file contents"
end
- it "should save a file" do
- ::File.expects(:exist?).with("#{@dir}/contents").returns false
- ::File.expects(:directory?).with(@dir).returns false
- ::FileUtils.expects(:mkdir_p).with(@dir)
- ::File.expects(:open).with("#{@dir}/contents",
::File::WRONLY|::File::CREAT, 0440)
-
- bucketfile = Puppet::FileBucket::File.new(@contents)
- bucketfile.save
-
- end
-
def make_bucketed_file
FileUtils.mkdir_p(@dir)
File.open("#{@dir}/contents", 'w') { |f| f.write @contents }
On Mon, Feb 21, 2011 at 1:39 PM, Paul Berry <[email protected]> wrote:
> Commit 2274d5104f6e413a2b8899a3c3111a17bbb2f4d7 optimized network
> usage for the case where a file is already in the filebucket.
> However, it took away the ability to store paths.
>
> This change restores the ability to store paths while maintaining
> optimal network usage for the case where the file is already in the
> filebucket with the given path. This is expected to be the most
> common case.
>
> Paired-with: Jesse Wolfe <[email protected]>
> Signed-off-by: Paul Berry <[email protected]>
> ---
> lib/puppet/file_bucket/dipper.rb | 5 +-
> lib/puppet/indirector/file_bucket_file/file.rb | 55 ++++++++---
> spec/unit/indirector/file_bucket_file/file_spec.rb | 109
> ++++++++++++++++++--
> 3 files changed, 144 insertions(+), 25 deletions(-)
>
> diff --git a/lib/puppet/file_bucket/dipper.rb
> b/lib/puppet/file_bucket/dipper.rb
> index f4bef28..de4c01b 100644
> --- a/lib/puppet/file_bucket/dipper.rb
> +++ b/lib/puppet/file_bucket/dipper.rb
> @@ -34,11 +34,12 @@ class Puppet::FileBucket::Dipper
> contents = ::File.read(file)
> begin
> file_bucket_file = Puppet::FileBucket::File.new(contents,
> :bucket_path => @local_path)
> - dest_path = "#{@rest_path}#{file_bucket_file.name}"
> + files_original_path = absolutize_path(file)
> + dest_path = "#{@rest_path}#{file_bucket_file.name
> }#{files_original_path}"
>
> # Make a HEAD request for the file so that we don't waste time
> # uploading it if it already exists in the bucket.
> - unless
> Puppet::FileBucket::File.head("#{@rest_path}#{file_bucket_file.checksum_type}/#{file_bucket_file.checksum_data}")
> + unless
> Puppet::FileBucket::File.head("#{@rest_path}#{file_bucket_file.checksum_type}/#{file_bucket_file.checksum_data}#{files_original_path}")
> file_bucket_file.save(dest_path)
> end
>
> diff --git a/lib/puppet/indirector/file_bucket_file/file.rb
> b/lib/puppet/indirector/file_bucket_file/file.rb
> index 8bea2d7..0fd8a91 100644
> --- a/lib/puppet/indirector/file_bucket_file/file.rb
> +++ b/lib/puppet/indirector/file_bucket_file/file.rb
> @@ -14,10 +14,12 @@ module Puppet::FileBucketFile
> end
>
> def find( request )
> - checksum = request_to_checksum( request )
> - file_path = path_for(request.options[:bucket_path], checksum,
> 'contents')
> + checksum, files_original_path = request_to_checksum_and_path(
> request )
> + dir_path = path_for(request.options[:bucket_path], checksum)
> + file_path = ::File.join(dir_path, 'contents')
>
> return nil unless ::File.exists?(file_path)
> + return nil unless path_match(dir_path, files_original_path)
>
> if request.options[:diff_with]
> hash_protocol = sumtype(checksum)
> @@ -32,32 +34,47 @@ module Puppet::FileBucketFile
> end
>
> def head(request)
> - checksum = request_to_checksum(request)
> - file_path = path_for(request.options[:bucket_path], checksum,
> 'contents')
> - ::File.exists?(file_path)
> + checksum, files_original_path =
> request_to_checksum_and_path(request)
> + dir_path = path_for(request.options[:bucket_path], checksum)
> +
> + ::File.exists?(::File.join(dir_path, 'contents')) and
> path_match(dir_path, files_original_path)
> end
>
> def save( request )
> instance = request.instance
> + checksum, files_original_path =
> request_to_checksum_and_path(request)
>
> - save_to_disk(instance)
> + save_to_disk(instance, files_original_path)
> instance.to_s
> end
>
> private
>
> - def save_to_disk( bucket_file )
> + def path_match(dir_path, files_original_path)
> + return true unless files_original_path # if no path was provided,
> it's a match
> + paths_path = ::File.join(dir_path, 'paths')
> + return false unless ::File.exists?(paths_path)
> + ::File.open(paths_path) do |f|
> + f.each do |line|
> + return true if line.chomp == files_original_path
> + end
> + end
> + return false
> + end
> +
> + def save_to_disk( bucket_file, files_original_path )
> filename = path_for(bucket_file.bucket_path,
> bucket_file.checksum_data, 'contents')
> - dirname = path_for(bucket_file.bucket_path,
> bucket_file.checksum_data)
> + dir_path = path_for(bucket_file.bucket_path,
> bucket_file.checksum_data)
> + paths_path = ::File.join(dir_path, 'paths')
>
> # If the file already exists, do nothing.
> if ::File.exist?(filename)
> verify_identical_file!(bucket_file)
> else
> # Make the directories if necessary.
> - unless ::File.directory?(dirname)
> + unless ::File.directory?(dir_path)
> Puppet::Util.withumask(0007) do
> - ::FileUtils.mkdir_p(dirname)
> + ::FileUtils.mkdir_p(dir_path)
> end
> end
>
> @@ -68,15 +85,27 @@ module Puppet::FileBucketFile
> ::File.open(filename, ::File::WRONLY|::File::CREAT, 0440) do |of|
> of.print bucket_file.contents
> end
> + ::File.open(paths_path, ::File::WRONLY|::File::CREAT, 0640) do
> |of|
> + # path will be written below
> + end
> + end
> + end
> +
> + unless path_match(dir_path, files_original_path)
> + ::File.open(paths_path, 'a') do |f|
> + f.puts(files_original_path)
> end
> end
> end
>
> - def request_to_checksum( request )
> - checksum_type, checksum, path = request.key.split(/\//, 3) # Note:
> we ignore path if present.
> + def request_to_checksum_and_path( request )
> + checksum_type, checksum, path = request.key.split(/\//, 3)
> + if path == '' # Treat "md5/<checksum>/" like "md5/<checksum>"
> + path = nil
> + end
> raise "Unsupported checksum type #{checksum_type.inspect}" if
> checksum_type != 'md5'
> raise "Invalid checksum #{checksum.inspect}" if checksum !~
> /^[0-9a-f]{32}$/
> - checksum
> + [checksum, path]
> end
>
> def path_for(bucket_path, digest, subfile = nil)
> diff --git a/spec/unit/indirector/file_bucket_file/file_spec.rb
> b/spec/unit/indirector/file_bucket_file/file_spec.rb
> index 9187f4d..0c33593 100755
> --- a/spec/unit/indirector/file_bucket_file/file_spec.rb
> +++ b/spec/unit/indirector/file_bucket_file/file_spec.rb
> @@ -22,13 +22,97 @@ describe Puppet::FileBucketFile::File do
> Puppet[:bucketdir] = tmpdir('bucketdir')
> end
>
> - describe "when diffing files" do
> - def save_bucket_file(contents)
> - bucket_file = Puppet::FileBucket::File.new(contents)
> - bucket_file.save
> - bucket_file.checksum_data
> + def save_bucket_file(contents, path = "/who_cares")
> + bucket_file = Puppet::FileBucket::File.new(contents)
> + bucket_file.save("md5/#{Digest::MD5.hexdigest(contents)}#{path}")
> + bucket_file.checksum_data
> + end
> +
> + describe "when servicing a save request" do
> + describe "when supplying a path" do
> + it "should store the path if not already stored" do
> + checksum = save_bucket_file("stuff", "/foo/bar")
> + dir_path =
> "#{Puppet[:bucketdir]}/c/1/3/d/8/8/c/b/c13d88cb4cb02003daedb8a84e5d272a"
> + File.read("#{dir_path}/contents").should == "stuff"
> + File.read("#{dir_path}/paths").should == "foo/bar\n"
> + end
> +
> + it "should leave the paths file alone if the path is already
> stored" do
> + checksum = save_bucket_file("stuff", "/foo/bar")
> + checksum = save_bucket_file("stuff", "/foo/bar")
> + dir_path =
> "#{Puppet[:bucketdir]}/c/1/3/d/8/8/c/b/c13d88cb4cb02003daedb8a84e5d272a"
> + File.read("#{dir_path}/contents").should == "stuff"
> + File.read("#{dir_path}/paths").should == "foo/bar\n"
> + end
> +
> + it "should store an additional path if the new path differs from
> those already stored" do
> + checksum = save_bucket_file("stuff", "/foo/bar")
> + checksum = save_bucket_file("stuff", "/foo/baz")
> + dir_path =
> "#{Puppet[:bucketdir]}/c/1/3/d/8/8/c/b/c13d88cb4cb02003daedb8a84e5d272a"
> + File.read("#{dir_path}/contents").should == "stuff"
> + File.read("#{dir_path}/paths").should == "foo/bar\nfoo/baz\n"
> + end
> + end
> +
> + describe "when not supplying a path" do
> + it "should save the file and create an empty paths file" do
> + checksum = save_bucket_file("stuff", "")
> + dir_path =
> "#{Puppet[:bucketdir]}/c/1/3/d/8/8/c/b/c13d88cb4cb02003daedb8a84e5d272a"
> + File.read("#{dir_path}/contents").should == "stuff"
> + File.read("#{dir_path}/paths").should == ""
> + end
> + end
> + end
> +
> + describe "when servicing a head/find request" do
> + describe "when supplying a path" do
> + it "should return false/nil if the file isn't bucketed" do
> +
>
> Puppet::FileBucket::File.head("md5/0ae2ec1980410229885fe72f7b44fe55/foo/bar").should
> == false
> +
>
> Puppet::FileBucket::File.find("md5/0ae2ec1980410229885fe72f7b44fe55/foo/bar").should
> == nil
> + end
> +
> + it "should return false/nil if the file is bucketed but with a
> different path" do
> + checksum = save_bucket_file("I'm the contents of a file",
> '/foo/bar')
> + Puppet::FileBucket::File.head("md5/#{checksum}/foo/baz").should
> == false
> + Puppet::FileBucket::File.find("md5/#{checksum}/foo/baz").should
> == nil
> + end
> +
> + it "should return true/file if the file is already bucketed with
> the given path" do
> + contents = "I'm the contents of a file"
> + checksum = save_bucket_file(contents, '/foo/bar')
> + Puppet::FileBucket::File.head("md5/#{checksum}/foo/bar").should
> == true
> + find_result =
> Puppet::FileBucket::File.find("md5/#{checksum}/foo/bar")
> + find_result.should be_a(Puppet::FileBucket::File)
> + find_result.checksum.should == "{md5}#{checksum}"
> + find_result.to_s.should == contents
> + end
> + end
> +
> + describe "when not supplying a path" do
> + [false, true].each do |trailing_slash|
> + describe "#{trailing_slash ? 'with' : 'without'} a trailing
> slash" do
> + trailing_string = trailing_slash ? '/' : ''
> +
> + it "should return false/nil if the file isn't bucketed" do
> +
>
> Puppet::FileBucket::File.head("md5/0ae2ec1980410229885fe72f7b44fe55#{trailing_string}").should
> == false
> +
>
> Puppet::FileBucket::File.find("md5/0ae2ec1980410229885fe72f7b44fe55#{trailing_string}").should
> == nil
> + end
> +
> + it "should return true/file if the file is already bucketed"
> do
> + contents = "I'm the contents of a file"
> + checksum = save_bucket_file(contents, '/foo/bar')
> +
> Puppet::FileBucket::File.head("md5/#{checksum}#{trailing_string}").should
> == true
> + find_result =
> Puppet::FileBucket::File.find("md5/#{checksum}#{trailing_string}")
> + find_result.should be_a(Puppet::FileBucket::File)
> + find_result.checksum.should == "{md5}#{checksum}"
> + find_result.to_s.should == contents
> + end
> + end
> + end
> end
> + end
>
> + describe "when diffing files" do
> it "should generate an empty string if there is no diff" do
> checksum = save_bucket_file("I'm the contents of a file")
> Puppet::FileBucket::File.find("md5/#{checksum}", :diff_with =>
> checksum).should == ''
> @@ -102,7 +186,7 @@ HERE
>
> key = "md5/#{@digest}"
> if supply_path
> - key += "//path/to/file"
> + key += "/path/to/file"
> end
>
> @request = Puppet::Indirector::Request.new(:indirection_name,
> :find, key, request_options)
> @@ -116,10 +200,15 @@ HERE
> it "should return an instance of Puppet::FileBucket::File
> created with the content if the file exists" do
> make_bucketed_file
>
> - bucketfile = @store.find(@request)
> - bucketfile.should be_a(Puppet::FileBucket::File)
> - bucketfile.contents.should == @contents
> - @store.head(@request).should == true
> + if supply_path
> + @store.find(@request).should == nil
> + @store.head(@request).should == false # because path
> didn't match
> + else
> + bucketfile = @store.find(@request)
> + bucketfile.should be_a(Puppet::FileBucket::File)
> + bucketfile.contents.should == @contents
> + @store.head(@request).should == true
> + end
> end
>
> it "should return nil if no file is found" do
> --
> 1.7.2
>
> --
> 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.
>
>
--
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.
