This switches the file's 'content' parameter to always use checksums, rather than always using content but switching to checksums whenever necessary.
This greatly simplifies all the logging requirements (so that content doesn't show up in logs), but also simplifies insync comparisons, and much more. In the process, I found that the code was pulling down file content more often than was necessary, and fixing that cut 40% off of the time of a very small transaction. Signed-off-by: Luke Kanies <[email protected]> --- lib/puppet/type/file/checksum.rb | 5 +- lib/puppet/type/file/content.rb | 57 ++++++++++----- lib/puppet/type/file/ensure.rb | 14 +++- spec/unit/type/file/content.rb | 155 ++++++++++++++++++++++++++------------ 4 files changed, 158 insertions(+), 73 deletions(-) diff --git a/lib/puppet/type/file/checksum.rb b/lib/puppet/type/file/checksum.rb index 3b74863..76e27e5 100755 --- a/lib/puppet/type/file/checksum.rb +++ b/lib/puppet/type/file/checksum.rb @@ -204,10 +204,7 @@ Puppet::Type.type(:file).newproperty(:checksum) do if currentvalue == :absent # if they're copying, then we won't worry about the file # not existing yet - unless @resource.property(:source) - self.warning("File %s does not exist -- cannot checksum" % @resource[:path]) - end - return nil + return nil unless @resource.property(:source) end # If the sums are different, then return an event. diff --git a/lib/puppet/type/file/content.rb b/lib/puppet/type/file/content.rb index 385a863..a5fe992 100755 --- a/lib/puppet/type/file/content.rb +++ b/lib/puppet/type/file/content.rb @@ -25,17 +25,44 @@ module Puppet This attribute is especially useful when used with `PuppetTemplating templating`:trac:." - def string_as_checksum(string) - return "absent" if string == :absent - "{md5}" + Digest::MD5.hexdigest(string) + # Store a checksum as the value, rather than the actual content. + # Simplifies everything. + munge do |value| + if value == :absent + value + else + @actual_content = value + "{#{checksum_type}}" + send(self.checksum_type, value) + end end - def should_to_s(should) - string_as_checksum(should) + def checksum_type + if source = resource.parameter(:source) + source.checksum =~ /^\{(\w+)\}.+/ + return $1.to_sym + elsif checksum = resource.parameter(:checksum) + result = checksum.checktype + if result =~ /^\{(\w+)\}.+/ + return $1.to_sym + else + return result + end + else + return :md5 + end end - def is_to_s(is) - string_as_checksum(is) + # If content was specified, return that; else try to return the source content; + # else, return nil. + def actual_content + if defined?(@actual_content) and @actual_content + return @actual_content + end + + if s = resource.parameter(:source) + return s.content + end + return nil end def content @@ -58,12 +85,7 @@ module Puppet return super elsif source = resource.parameter(:source) fail "Got a remote source with no checksum" unless source.checksum - unless sum_method = sumtype(source.checksum) - fail "Could not extract checksum type from source checksum '%s'" % source.checksum - end - - newsum = "{%s}" % sum_method + send(sum_method, is) - result = (newsum == source.checksum) + result = (is == source.checksum) else # We've got no content specified, and no source from which to # get content. @@ -71,7 +93,7 @@ module Puppet end if ! result and Puppet[:show_diff] - string_file_diff(@resource[:path], content) + string_file_diff(@resource[:path], actual_content) end return result end @@ -83,7 +105,7 @@ module Puppet return nil if stat.ftype == "directory" begin - return File.read(@resource[:path]) + return "{#{checksum_type}}" + send(checksum_type.to_s + "_file", resource[:path]) rescue => detail raise Puppet::Error, "Could not read %s: %s" % [[email protected], detail] end @@ -91,8 +113,8 @@ module Puppet # Make sure we're also managing the checksum property. def should=(value) - super @resource.newattr(:checksum) unless @resource.parameter(:checksum) + super end # Just write our content out to disk. @@ -102,8 +124,7 @@ module Puppet # We're safe not testing for the 'source' if there's no 'should' # because we wouldn't have gotten this far if there weren't at least # one valid value somewhere. - content = self.should || resource.parameter(:source).content - @resource.write(content, :content) + @resource.write(actual_content, :content) return return_event end diff --git a/lib/puppet/type/file/ensure.rb b/lib/puppet/type/file/ensure.rb index 7466c5e..5c4d98d 100755 --- a/lib/puppet/type/file/ensure.rb +++ b/lib/puppet/type/file/ensure.rb @@ -110,11 +110,19 @@ module Puppet end def change_to_s(currentvalue, newvalue) - if property = @resource.property(:content) and content = property.retrieve and ! property.insync?(content) - return property.change_to_s(content, property.should) + return super unless newvalue.to_s == "file" + + return super unless property = @resource.property(:content) + + # We know that content is out of sync if we're here, because + # it's essentially equivalent to 'ensure' in the transaction. + if source = @resource.parameter(:source) + should = source.checksum else - super(currentvalue, newvalue) + should = property.should end + + return property.change_to_s(property.retrieve, should) end # Check that we can actually create anything diff --git a/spec/unit/type/file/content.rb b/spec/unit/type/file/content.rb index 212bb2f..fd225fa 100755 --- a/spec/unit/type/file/content.rb +++ b/spec/unit/type/file/content.rb @@ -13,6 +13,95 @@ describe content do content.superclass.must == Puppet::Property end + describe "when determining the checksum type" do + it "should use the type specified in the source checksum if a source is set" do + source = mock 'source' + source.expects(:checksum).returns "{litemd5}eh" + + @resource.expects(:parameter).with(:source).returns source + + @content = content.new(:resource => @resource) + @content.checksum_type.should == :litemd5 + end + + it "should use the type specified by the checksum parameter if no source is set" do + checksum = mock 'checksum' + checksum.expects(:checktype).returns :litemd5 + + @resource.expects(:parameter).with(:source).returns nil + @resource.expects(:parameter).with(:checksum).returns checksum + + @content = content.new(:resource => @resource) + @content.checksum_type.should == :litemd5 + end + + it "should only return the checksum type from the checksum parameter if the parameter returns a whole checksum" do + checksum = mock 'checksum' + checksum.expects(:checktype).returns "{md5}something" + + @resource.expects(:parameter).with(:source).returns nil + @resource.expects(:parameter).with(:checksum).returns checksum + + @content = content.new(:resource => @resource) + @content.checksum_type.should == :md5 + end + + it "should use md5 if neither a source nor a checksum parameter is available" do + @content = content.new(:resource => @resource) + @content.checksum_type.should == :md5 + end + end + + describe "when determining the actual content to write" do + it "should use the set content if available" do + @content = content.new(:resource => @resource) + @content.should = "ehness" + @content.actual_content.should == "ehness" + end + + it "should use the content from the source if the source is set" do + source = mock 'source' + source.expects(:content).returns "scont" + + @resource.expects(:parameter).with(:source).returns source + + @content = content.new(:resource => @resource) + @content.actual_content.should == "scont" + end + + it "should return nil if no source is available and no content is set" do + @content = content.new(:resource => @resource) + @content.actual_content.should be_nil + end + end + + describe "when setting the desired content" do + it "should make the actual content available via an attribute" do + @content = content.new(:resource => @resource) + @content.stubs(:checksum_type).returns "md5" + @content.should = "this is some content" + + @content.actual_content.should == "this is some content" + end + + it "should store the checksum as the desired content" do + @content = content.new(:resource => @resource) + digest = Digest::MD5.hexdigest("this is some content") + + @content.stubs(:checksum_type).returns "md5" + @content.should = "this is some content" + + @content.should.must == "{md5}#{digest}" + end + + it "should not checksum 'absent'" do + @content = content.new(:resource => @resource) + @content.should = :absent + + @content.should.must == :absent + end + end + describe "when retrieving the current content" do it "should return :absent if the file does not exist" do @content = content.new(:resource => @resource) @@ -21,9 +110,7 @@ describe content do @content.retrieve.should == :absent end - it "should not manage content on non-files" do - pending "Haven't decided how this should behave" - + it "should not manage content on directories" do @content = content.new(:resource => @resource) stat = mock 'stat', :ftype => "directory" @@ -32,16 +119,18 @@ describe content do @content.retrieve.should be_nil end - it "should return the current content of the file if it exists and is a normal file" do + it "should return the checksum of the file if it exists and is a normal file" do @content = content.new(:resource => @resource) + @content.stubs(:checksum_type).returns "md5" stat = mock 'stat', :ftype => "file" @resource.expects(:stat).returns stat @resource.expects(:[]).with(:path).returns "/my/file" - File.expects(:read).with("/my/file").returns "some content" - @content.retrieve.should == "some content" + @content.expects(:md5_file).with("/my/file").returns "mysum" + + @content.retrieve.should == "{md5}mysum" end end @@ -51,7 +140,7 @@ describe content do @resource.stubs(:replace?).returns true @resource.stubs(:should_be_file?).returns true @content = content.new(:resource => @resource) - @content.should = "something" + @content.stubs(:checksum_type).returns "md5" end it "should return true if the resource shouldn't be a regular file" do @@ -79,9 +168,9 @@ describe content do @content.should_not be_insync("other content") end - it "should return true if the current contents are the same as the desired content" do + it "should return true if the sum for the current contents is the same as the sum for the desired content" do @content.should = "some content" - @content.must be_insync("some content") + @content.must be_insync("{md5}" + Digest::MD5.hexdigest("some content")) end describe "and the content is specified via a remote source" do @@ -89,15 +178,12 @@ describe content do @metadata = stub 'metadata' @source = stub 'source', :metadata => @metadata @resource.stubs(:parameter).with(:source).returns @source - - @content.should = nil end it "should use checksums to compare remote content, rather than downloading the content" do - @content.expects(:md5).with("some content").returns "whatever" @source.stubs(:checksum).returns "{md5}whatever" - @content.insync?("some content") + @content.insync?("{md5}eh") end it "should return false if the current content is different from the remote content" do @@ -107,10 +193,9 @@ describe content do end it "should return true if the current content is the same as the remote content" do - sum = @content.md5("some content") - @source.stubs(:checksum).returns("{md5}%s" % sum) + @source.stubs(:checksum).returns("{md5}something") - @content.must be_insync("some content") + @content.must be_insync("{md5}something") end end end @@ -141,54 +226,28 @@ describe content do describe "when changing the content" do before do @content = content.new(:resource => @resource) + @content.should = "some content" @resource.stubs(:[]).with(:path).returns "/boo" + @resource.stubs(:stat).returns "eh" end it "should use the file's :write method to write the content" do - pending "not switched from :source yet" - @resource.expects(:write).with("foobar", :content, 123) + @resource.expects(:write).with("some content", :content) @content.sync end it "should return :file_changed if the file already existed" do - pending "not switched from :source yet" + @resource.expects(:stat).returns "something" @resource.stubs(:write) - FileTest.expects(:exist?).with("/boo").returns true @content.sync.should == :file_changed end - it "should return :file_created if the file already existed" do - pending "not switched from :source yet" + it "should return :file_created if the file did not exist" do + @resource.expects(:stat).returns nil @resource.stubs(:write) - FileTest.expects(:exist?).with("/boo").returns false @content.sync.should == :file_created end end - - describe "when logging changes" do - before do - @resource = stub 'resource', :line => "foo", :file => "bar", :replace? => true - @resource.stubs(:[]).returns "foo" - @resource.stubs(:[]).with(:path).returns "/my/file" - @content = content.new :resource => @resource - end - - it "should not include current contents" do - @content.change_to_s("current_content", "desired").should_not be_include("current_content") - end - - it "should not include desired contents" do - @content.change_to_s("current", "desired_content").should_not be_include("desired_content") - end - - it "should not include the content when converting current content to a string" do - @content.is_to_s("my_content").should_not be_include("my_content") - end - - it "should not include the content when converting desired content to a string" do - @content.should_to_s("my_content").should_not be_include("my_content") - end - end end -- 1.6.1 --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
