Please review pull request #251: Don't try to read the body of a file content response twice opened by (nicklewis)
Description:
Previously, get_file_from_source was calling Net::HTTP#request_get to
request file content, and yielding the response to
chunk_file_from_source, which would read the body. However, when called
without a block, request_get will read the body itself before returning
the response. This caused an error when chunk_file_from_source then
tried to read (and inflate if necessary) the body.
Now, get_file_from_source will pass its block along to request_get,
which will yield the unread response, rather than yielding the result of
request_get, which is a read response. This prevents a double read
error, and also allows us to properly inflate the body.
This fixes sourcing remote files.
- Opened: Wed Dec 07 21:47:19 UTC 2011
- Based on: puppetlabs:master (3406081b3d91bb796f8fa666d22a05054fda42f4)
- Requested merge: nicklewis:fix-read_body-called-twice (491b6e39c8bd4ee89e45f71b9e608541a14cd592)
Diff follows:
diff --git a/acceptance/tests/resource/file/source_attribtute.rb b/acceptance/tests/resource/file/source_attribtute.rb
deleted file mode 100644
index 3fa447a..0000000
--- a/acceptance/tests/resource/file/source_attribtute.rb
+++ /dev/null
@@ -1,106 +0,0 @@
-test_name "The source attribute"
-
-step "Ensure the test environment is clean"
-on agents, 'rm -f /tmp/source_file_test.txt'
-
-# TODO: Add tests for puppet:// URIs with master/agent setups.
-# step "when using a puppet:/// URI with a master/agent setup"
-# step "when using a puppet://$server/ URI with a master/agent setup"
-
-step "Using a local file path"
-on agents, "echo 'Yay, this is the local file.' > /tmp/local_source_file_test.txt"
-manifest = "file { '/tmp/source_file_test.txt': source => '/tmp/local_source_file_test.txt', ensure => present }"
-
-apply_manifest_on agents, manifest
-agents.each do |host|
- on host, "cat /tmp/source_file_test.txt" do
- assert_match(/Yay, this is the local file./, stdout, "FIRST: File contents not matched on #{host}")
- end
-end
-
-step "Ensure the test environment is clean"
-on agents, 'rm -f /tmp/source_file_test.txt'
-
-step "Using a puppet:/// URI with puppet apply"
-
-on agents, puppet_agent("--configprint modulepath") do
- modulepath = stdout.split(':')[0]
- modulepath = modulepath.chomp
- on agents, "mkdir -p #{modulepath}/test_module/files"
- #on agents, "echo 'Yay, this is the puppet:/// file.' > #{modulepath}/test_module/files/test_file.txt"
- on agents, "echo 'Yay, this is the puppetfile.' > #{modulepath}/test_module/files/test_file.txt"
-end
-
-on agents, %q{echo "file { '/tmp/source_file_test.txt': source => 'puppet:///modules/test_module/test_file.txt', ensure => present }" > /tmp/source_test_manifest.pp}
-on agents, puppet_apply("/tmp/source_test_manifest.pp")
-
-agents.each do |host|
- on host, "cat /tmp/source_file_test.txt" do
- assert_match(/Yay, this is the puppetfile./, stdout, "SECOND: File contents not matched on #{host}")
- end
-end
-
-# Oops. We (Jesse & Jacob) ended up writing this before realizing that you
-# can't actually specify "source => 'http://...'". However, we're leaving it
-# here, since there have been feature requests to support doing this.
-# -- Mon, 07 Mar 2011 16:12:56 -0800
-#
-#step "Ensure the test environment is clean"
-#on agents, 'rm -f /tmp/source_file_test.txt'
-#
-#step "when using an http:// file path"
-#
-#File.open '/tmp/puppet-acceptance-webrick-script.rb', 'w' do |file|
-# file.puts %q{#!/usr/bin/env ruby
-#
-#require 'webrick'
-#
-#class Simple < WEBrick::HTTPServlet::AbstractServlet
-# def do_GET(request, response)
-# status, content_type, body = do_stuff_with(request)
-#
-# response.status = status
-# response['Content-Type'] = content_type
-# response.body = body
-# end
-#
-# def do_stuff_with(request)
-# return 200, "text/plain", "you got a page"
-# end
-#end
-#
-#class SimpleTwo < WEBrick::HTTPServlet::AbstractServlet
-# def do_GET(request, response)
-# status, content_type, body = do_stuff_with(request)
-#
-# response.status = status
-# response['Content-Type'] = content_type
-# response.body = body
-# end
-#
-# def do_stuff_with(request)
-# return 200, "text/plain", "you got a different page"
-# end
-#end
-#
-#server = WEBrick::HTTPServer.new :Port => 8081
-#trap "INT" do server.shutdown end
-#trap "TERM" do server.shutdown end
-#trap "QUIT" do server.shutdown end
-#
-#server.mount "/", SimpleTwo
-#server.mount "/foo.txt", Simple
-#server.start
-#}
-#end
-#
-#scp_to master, '/tmp/puppet-acceptance-webrick-script.rb', '/tmp'
-#on master, "chmod +x /tmp/puppet-acceptance-webrick-script.rb && /tmp/puppet-acceptance-webrick-script.rb &"
-#
-#manifest = "file { '/tmp/source_file_test.txt': source => 'http://#{master}:8081/foo.txt', ensure => present }"
-#
-#apply_manifest_on agents, manifest
-#
-#on agents, 'test "$(cat /tmp/source_file_test.txt)" = "you got a page"'
-#
-#on master, "killall puppet-acceptance-webrick-script.rb"
diff --git a/acceptance/tests/resource/file/source_attribute.rb b/acceptance/tests/resource/file/source_attribute.rb
new file mode 100644
index 0000000..c406c32
--- /dev/null
+++ b/acceptance/tests/resource/file/source_attribute.rb
@@ -0,0 +1,127 @@
+test_name "The source attribute"
+
+step "Ensure the test environment is clean"
+on agents, 'rm -f /tmp/source_file_test.txt'
+
+step "when using a puppet:/// URI with a master/agent setup"
+modulepath = nil
+on master, puppet_master('--configprint modulepath') do
+ modulepath = stdout.split(':')[0].chomp
+end
+
+result_file = "/tmp/#{$$}-result-file"
+source_file = File.join(modulepath, 'source_test_module', 'files', 'source_file')
+manifest = "/tmp/#{$$}-source-test.pp"
+
+on master, "mkdir -p #{File.dirname(source_file)}"
+on master, "echo 'the content is present' > #{source_file}"
+on master, %Q[echo "file { '#{result_file}': source => 'puppet:///modules/source_test_module/source_file', ensure => present }" > #{source_file}]
+
+with_master_running_on master, "--autosign true --manifest #{manifest}" do
+ run_agent_on agents, "--test --server #{master}" do
+ on agents, "cat #{result_file}" do
+ assert_match(/the content is present/, stdout, "Result file not created")
+ end
+ end
+end
+
+# TODO: Add tests for puppet:// URIs with multi-master/agent setups.
+# step "when using a puppet://$server/ URI with a master/agent setup"
+
+step "Using a local file path"
+on agents, "echo 'Yay, this is the local file.' > /tmp/local_source_file_test.txt"
+manifest = "file { '/tmp/source_file_test.txt': source => '/tmp/local_source_file_test.txt', ensure => present }"
+
+apply_manifest_on agents, manifest
+agents.each do |host|
+ on host, "cat /tmp/source_file_test.txt" do
+ assert_match(/Yay, this is the local file./, stdout, "FIRST: File contents not matched on #{host}")
+ end
+end
+
+step "Ensure the test environment is clean"
+on agents, 'rm -f /tmp/source_file_test.txt'
+
+step "Using a puppet:/// URI with puppet apply"
+
+on agents, puppet_agent("--configprint modulepath") do
+ modulepath = stdout.split(':')[0]
+ modulepath = modulepath.chomp
+ on agents, "mkdir -p #{modulepath}/test_module/files"
+ #on agents, "echo 'Yay, this is the puppet:/// file.' > #{modulepath}/test_module/files/test_file.txt"
+ on agents, "echo 'Yay, this is the puppetfile.' > #{modulepath}/test_module/files/test_file.txt"
+end
+
+on agents, %q{echo "file { '/tmp/source_file_test.txt': source => 'puppet:///modules/test_module/test_file.txt', ensure => present }" > /tmp/source_test_manifest.pp}
+on agents, puppet_apply("/tmp/source_test_manifest.pp")
+
+agents.each do |host|
+ on host, "cat /tmp/source_file_test.txt" do
+ assert_match(/Yay, this is the puppetfile./, stdout, "SECOND: File contents not matched on #{host}")
+ end
+end
+
+# Oops. We (Jesse & Jacob) ended up writing this before realizing that you
+# can't actually specify "source => 'http://...'". However, we're leaving it
+# here, since there have been feature requests to support doing this.
+# -- Mon, 07 Mar 2011 16:12:56 -0800
+#
+#step "Ensure the test environment is clean"
+#on agents, 'rm -f /tmp/source_file_test.txt'
+#
+#step "when using an http:// file path"
+#
+#File.open '/tmp/puppet-acceptance-webrick-script.rb', 'w' do |file|
+# file.puts %q{#!/usr/bin/env ruby
+#
+#require 'webrick'
+#
+#class Simple < WEBrick::HTTPServlet::AbstractServlet
+# def do_GET(request, response)
+# status, content_type, body = do_stuff_with(request)
+#
+# response.status = status
+# response['Content-Type'] = content_type
+# response.body = body
+# end
+#
+# def do_stuff_with(request)
+# return 200, "text/plain", "you got a page"
+# end
+#end
+#
+#class SimpleTwo < WEBrick::HTTPServlet::AbstractServlet
+# def do_GET(request, response)
+# status, content_type, body = do_stuff_with(request)
+#
+# response.status = status
+# response['Content-Type'] = content_type
+# response.body = body
+# end
+#
+# def do_stuff_with(request)
+# return 200, "text/plain", "you got a different page"
+# end
+#end
+#
+#server = WEBrick::HTTPServer.new :Port => 8081
+#trap "INT" do server.shutdown end
+#trap "TERM" do server.shutdown end
+#trap "QUIT" do server.shutdown end
+#
+#server.mount "/", SimpleTwo
+#server.mount "/foo.txt", Simple
+#server.start
+#}
+#end
+#
+#scp_to master, '/tmp/puppet-acceptance-webrick-script.rb', '/tmp'
+#on master, "chmod +x /tmp/puppet-acceptance-webrick-script.rb && /tmp/puppet-acceptance-webrick-script.rb &"
+#
+#manifest = "file { '/tmp/source_file_test.txt': source => 'http://#{master}:8081/foo.txt', ensure => present }"
+#
+#apply_manifest_on agents, manifest
+#
+#on agents, 'test "$(cat /tmp/source_file_test.txt)" = "you got a page"'
+#
+#on master, "killall puppet-acceptance-webrick-script.rb"
diff --git a/lib/puppet/type/file/content.rb b/lib/puppet/type/file/content.rb
index 81edeca..f68ed95 100755
--- a/lib/puppet/type/file/content.rb
+++ b/lib/puppet/type/file/content.rb
@@ -199,12 +199,12 @@ def chunk_file_from_disk(source_or_content)
end
end
- def get_from_source(source_or_content)
+ def get_from_source(source_or_content, &block)
request = Puppet::Indirector::Request.new(:file_content, :find, source_or_content.full_path.sub(/^\//,''))
request.do_request(:fileserver) do |req|
connection = Puppet::Network::HttpPool.http_instance(req.server, req.port)
- return yield connection.request_get(indirection2uri(req), add_accept_encoding({"Accept" => "raw"}))
+ connection.request_get(indirection2uri(req), add_accept_encoding({"Accept" => "raw"}), &block)
end
end
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to puppet-dev@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.
