On 03/04/10 21:10, Markus Roberts wrote:
> (continued)
>
> diff --git a/lib/puppet/type/file/checksum.rb
>> b/lib/puppet/type/file/checksum.rb
>> index 5af943e..5b99bef 100755
>> --- a/lib/puppet/type/file/checksum.rb
>> +++ b/lib/puppet/type/file/checksum.rb
>> @@ -23,4 +23,11 @@ Puppet::Type.type(:file).newparam(:checksum) do
>> method = type.to_s + "_file"
>> "{#{type}}" + send(method, path).to_s
>> end
>> +
>> + def sum_stream(&block)
>> + type = value || :md5 # same comment as above
>> + method = type.to_s + "_stream"
>> + checksum = send(method, &block)
>> + "{#{type}}#{checksum}"
>> + end
>> end
>> diff --git a/lib/puppet/type/file/content.rb
>> b/lib/puppet/type/file/content.rb
>> index b6c671b..3789900 100755
>> --- a/lib/puppet/type/file/content.rb
>> +++ b/lib/puppet/type/file/content.rb
>> @@ -1,9 +1,14 @@
>> +require 'net/http'
>> +require 'uri'
>> +
>> require 'puppet/util/checksums'
>> +require 'puppet/network/http/api/v1'
>>
>> module Puppet
>> Puppet::Type.type(:file).newproperty(:content) do
>> include Puppet::Util::Diff
>> include Puppet::Util::Checksums
>> + include Puppet::Network::HTTP::API::V1
>>
>> desc "Specify the contents of a file as a string. Newlines, tabs,
>> and
>> spaces can be specified using the escaped syntax (e.g., \\n for
>> a
>> @@ -68,21 +73,22 @@ module Puppet
>> end
>> end
>>
>> - # If content was specified, return that; else try to return the
>> source content;
>> + # If content was specified, return that;
>> # 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
>> - fail "Could not find actual content from checksum"
>> + return nil
>> + end
>> +
>>
>
> :-) So that would be:
>
> def actual_content
> @actual_content
> end
I left it like it was so that your code smell automated refactoring
would find it :-D
> or even
>
> attr_reader :actual_content
Yes, definitely better :-)
>
>> + def length
>> + (actual_content and actual_content.length) || 0
>> end
>>
>> def content
>> - self.should || (s = resource.parameter(:source) and s.content)
>> + self.should
>> end
>>
>> # Override this method to provide diffs if asked for.
>> @@ -140,9 +146,62 @@ 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.
>> - @resource.write(actual_content, :content)
>> + @resource.write(:content)
>>
>> return return_event
>> end
>> +
>> + def write(file)
>> + return write_content(file) if actual_content
>> + return write_source(file) if resource.parameter(:source)
>> + self.fail "Writing content that wasn't provided"
>> + end
>> +
>> + def write_content(file)
>> + file.print actual_content
>> + return resource.parameter(:checksum).sum(actual_content)
>> + end
>> +
>> + def write_source(file)
>> + if source = resource.parameter(:source)
>> + return write_local_source(file, source) if source.local?
>> + return write_remote_source(file, source)
>> + end
>> + end
>> +
>> + def write_local_source(file, source)
>> + resource.parameter(:checksum).sum_stream do |sum|
>> + File.open(source.full_path, "r") do |src|
>> + puts "local open"
>>
>
> I assume the puts should be removed?
Oops.
>> + while chunk = src.read(8192)
>> + sum << chunk
>> + file.print chunk
>> + end
>> + end
>> + end
>> + end
>> +
>> + def write_remote_source(file, source)
>> + request = Puppet::Indirector::Request.new(:file_content,
>> :find, source.full_path)
>> + checksum = nil
>> + Puppet::Network::HttpPool.http_instance(source.server,
>> source.port).request_get(indirection2uri(request), {"Accept" => "raw"}) do
>> |response|
>> + case response.code
>> + when "404"
>> + return nil
>> + when /^2/
>> + checksum = resource.parameter(:checksum).sum_stream do
>> |sum|
>> + response.read_body do |chunk|
>> + sum << chunk
>> + file.print chunk
>> + end
>>
> + end
>> + else
>> + # Raise the http error if we didn't get a 'success' of
>> some kind.
>> + message = "Error %s on SERVER: %s" % [response.code,
>> (response.body||'').empty? ? response.message : response.body]
>> + raise Net::HTTPError.new(message, response)
>> + end
>> + end
>> + return checksum
>> + end
>> end
>> end
>>
>
> Hmmm. More thinking than recommending, but what about something like:
>
> def write_source(file)
> resource.parameter(:checksum).sum_stream { |sum|
> each_chunk_from(file,resource.parameter(:source) { |chunk|
> sum << chunk
> file.print chunk
> }
> }
> end
>
> def each_chunk_from(file,source)
> if source.nil?
> nil
> elsif source.local?
> File.open(source.full_path, "r") do |src|
> while chunk = src.read(8192)
> yield chunk
> end
> end
> else
> request = Puppet::Indirector::Request.new(:file_content,
> :find, source.full_path)
> connection =
> Puppet::Network::HttpPool.http_instance(source.server, source.port).
> connection.request_get(indirection2uri(request), {"Accept"
> => "raw"}) do |response|
> case response.code
> when "404"; nil
> when /^2/; response.read_body { |chunk| yield chunk }
> else
> # Raise the http error if we didn't get a 'success'
> of some kind.
> message = "Error %s on SERVER: %s" % [response.code,
> (response.body||'').empty? ? response.message : response.body]
> raise Net::HTTPError.new(message, response)
> end
> }
> end
> end
>
> In other words, rather than pushing the checksuming and writing into the
> walk the stream code, keep it at the write_source level and push the
> local/remote choice down instead? If this direction is good, it looks like
> we could go even further and unify write_source and write_content, by adding
> the content logic at the head of each_chunk_from.
Yes, that should work fine, but you'll have to wait next week-end for
this as I'll be AFK most of this week.
>
>> diff --git a/lib/puppet/type/file/ensure.rb
>> b/lib/puppet/type/file/ensure.rb
>> index 5f0beee..b8a85f6 100755
>> --- a/lib/puppet/type/file/ensure.rb
>> +++ b/lib/puppet/type/file/ensure.rb
>> @@ -45,7 +45,7 @@ module Puppet
>> if property = @resource.property(:content)
>> property.sync
>> else
>> - @resource.write("", :ensure)
>> + @resource.write(:ensure)
>> mode = @resource.should(:mode)
>> end
>> return :file_created
>> diff --git a/lib/puppet/type/file/source.rb
>> b/lib/puppet/type/file/source.rb
>> index 838dabb..5b86e82 100755
>> --- a/lib/puppet/type/file/source.rb
>> +++ b/lib/puppet/type/file/source.rb
>> @@ -96,16 +96,6 @@ module Puppet
>> metadata && metadata.checksum
>> end
>>
>> - # Look up (if necessary) and return remote content.
>> - cached_attr(:content) do
>> - raise Puppet::DevError, "No source for content was stored with
>> the metadata" unless metadata.source
>> -
>> - unless tmp =
>> Puppet::FileServing::Content.find(metadata.source)
>> - fail "Could not find any content at %s" % metadata.source
>> - end
>> - tmp.content
>> - end
>> -
>> # Copy the values from the source to the resource. Yay.
>> def copy_source_values
>> devfail "Somehow got asked to copy source values without any
>> metadata" unless metadata
>> @@ -171,5 +161,32 @@ module Puppet
>> resource[:check] = checks
>> resource[:checksum] = :md5 unless resource.property(:checksum)
>> end
>> +
>> + def local?
>> + if found? and uri
>> + return uri.scheme == "file" || uri.scheme == nil
>> + end
>> + false
>> + end
>>
>
> How about this (reading "||" as "defaults to"):
>
> def local?
> found? and uri and (uri.scheme || "file") == "file"
> end
I'm always amazed by this kind of construction :-)
Note that I don't find it readable...
>
>> + def full_path
>> + if found? and uri
>> + return URI.unescape(uri.path)
>> + end
>> + end
>> +
>> + def server
>> + (uri and uri.host) or Puppet.settings[:server]
>> + end
>> +
>> + def port
>> + (uri and uri.port) or Puppet.settings[:masterport]
>> + end
>> +
>> + private
>> +
>> + def uri
>> + @uri ||= URI.parse(URI.escape(metadata.source))
>> + end
>> end
>> end
>> diff --git a/lib/puppet/util/checksums.rb b/lib/puppet/util/checksums.rb
>> index b0dcf80..a7aeaea 100644
>> --- a/lib/puppet/util/checksums.rb
>> +++ b/lib/puppet/util/checksums.rb
>> @@ -39,11 +39,27 @@ module Puppet::Util::Checksums
>> md5_file(filename, true)
>> end
>>
>> + def md5_stream(&block)
>> + require 'digest/md5'
>> + digest = Digest::MD5.new()
>> + yield digest
>> + return digest.hexdigest
>> + end
>> +
>> + alias :md5lite_stream :md5_stream
>> +
>> # Return the :mtime timestamp of a file.
>> def mtime_file(filename)
>> File.stat(filename).send(:mtime)
>> end
>>
>> + # by definition this doesn't exist
>> + def mtime_stream
>> + nil
>> + end
>> +
>> + alias :ctime_stream :mtime_stream
>> +
>> # Calculate a checksum using Digest::SHA1.
>> def sha1(content)
>> require 'digest/sha1'
>> @@ -68,6 +84,15 @@ module Puppet::Util::Checksums
>> sha1_file(filename, true)
>> end
>>
>> + def sha1_stream
>> + require 'digest/sha1'
>> + digest = Digest::SHA1.new()
>> + yield digest
>> + return digest.hexdigest
>> + end
>> +
>> + alias :sha1lite_stream :sha1_stream
>> +
>> # Return the :ctime of a file.
>> def ctime_file(filename)
>> File.stat(filename).send(:ctime)
>>
>
>
>
>
> Final thought: I think this has the most thorough test set of any commit
> I've seen this year.
That's certainly because I didn't produce many patches this year :-D
--
Brice Figureau
My Blog: http://www.masterzen.fr/
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-...@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
