Merge pull request #5327 from chef/lcg/file-tempfile-preserves-extensions

preserve the extension of the file in the rendered tempfile in File providers

2 files changed, 43 insertions, 11 deletions

diff --git a/lib/chef/file_content_management/tempfile.rb b/lib/chef/file_content_management/tempfile.rb
index bba4b34d82..eb9154c4b1 100644
--- a/lib/chef/file_content_management/tempfile.rb
+++ b/lib/chef/file_content_management/tempfile.rb
@@ -40,7 +40,8 @@ class Chef
 
         tempfile_dirnames.each do |tempfile_dirname|
           begin
-            tf = ::Tempfile.open(tempfile_basename, tempfile_dirname)
+            # preserving the file extension of the target filename should be considered a public API
+            tf = ::Tempfile.open([tempfile_basename, tempfile_extension], tempfile_dirname)
             break
           rescue SystemCallError => e
             message = "Creating temp file under '#{tempfile_dirname}' failed with: '#{e.message}'"
@@ -63,12 +64,18 @@ class Chef
       # as the arguments to Tempfile.new() consistently.
       #
       def tempfile_basename
-        basename = ::File.basename(@new_resource.name)
+        basename = ::File.basename(@new_resource.path, tempfile_extension)
+        # the leading "[.]chef-" here should be considered a public API and should not be changed
         basename.insert 0, "chef-"
         basename.insert 0, "." unless Chef::Platform.windows? # dotfile if we're not on windows
         basename
       end
 
+      # this is similar to File.extname() but greedy about the extension (from the first dot, not the last dot)
+      def tempfile_extension
+        File.basename(@new_resource.path)[/\..*/] || ""
+      end
+
       # Returns the possible directories for the tempfile to be created in.
       def tempfile_dirnames
         # in why-run mode we need to create a Tempfile to compare against, which we will never
diff --git a/spec/unit/file_content_management/tempfile_spec.rb b/spec/unit/file_content_management/tempfile_spec.rb
index 4b27fc8cc9..b57b1ae8e1 100644
--- a/spec/unit/file_content_management/tempfile_spec.rb
+++ b/spec/unit/file_content_management/tempfile_spec.rb
@@ -19,38 +19,54 @@
 require "spec_helper"
 
 describe Chef::FileContentManagement::Tempfile do
-  let(:resource) do
-    r = Chef::Resource::File.new("new_file")
-    r.path "/foo/bar/new_file"
-    r
-  end
 
-  subject { described_class.new(resource) }
+  def tempfile_object_for_path(path)
+    r = Chef::Resource::File.new("decorative name that should not matter")
+    r.path path
+    Chef::FileContentManagement::Tempfile.new(r)
+  end
 
   describe "#tempfile_basename" do
     it "should return a dotfile", :unix_only do
+      subject = tempfile_object_for_path("/foo/bar/new_file")
       expect(subject.send(:tempfile_basename)).to eql(".chef-new_file")
     end
 
     it "should return a file", :windows_only do
+      subject = tempfile_object_for_path("/foo/bar/new_file")
       expect(subject.send(:tempfile_basename)).to eql("chef-new_file")
     end
+
+    it "should strip the extension" do
+      subject = tempfile_object_for_path("/foo/bar/new_file.html.erb")
+      expect(subject.send(:tempfile_basename)).to eql(".chef-new_file")
+    end
+  end
+
+  describe "#tempfile_extension" do
+    it "should preserve the file extension" do
+      subject = tempfile_object_for_path("/foo/bar/new_file.html.erb")
+      expect(subject.send(:tempfile_extension)).to eql(".html.erb")
+    end
   end
 
   describe "#tempfile_dirnames" do
 
     it "should select a temp dir" do
+      subject = tempfile_object_for_path("/foo/bar/new_file")
       Chef::Config[:file_staging_uses_destdir] = false
       expect(Dir).to receive(:tmpdir).and_return("/tmp/dir")
       expect(subject.send(:tempfile_dirnames)).to eql(%w{ /tmp/dir })
     end
 
     it "should select the destdir" do
+      subject = tempfile_object_for_path("/foo/bar/new_file")
       Chef::Config[:file_staging_uses_destdir] = true
       expect(subject.send(:tempfile_dirnames)).to eql(%w{ /foo/bar })
     end
 
     it "should select the destdir and a temp dir" do
+      subject = tempfile_object_for_path("/foo/bar/new_file")
       Chef::Config[:file_staging_uses_destdir] = :auto
       expect(Dir).to receive(:tmpdir).and_return("/tmp/dir")
       expect(subject.send(:tempfile_dirnames)).to eql(%w{ /foo/bar /tmp/dir })
@@ -67,18 +83,27 @@ describe Chef::FileContentManagement::Tempfile do
     end
 
     it "should create a temporary file" do
+      subject = tempfile_object_for_path("/foo/bar/new_file")
       expect(subject.send(:tempfile_open)).to be_a(Tempfile)
     end
 
+    it "should preserve the extension in the tempfile path" do
+      subject = tempfile_object_for_path("/foo/bar/new_file.html.erb")
+      tempfile = subject.send(:tempfile_open)
+      expect(tempfile.path).to match(/chef-new_file.*\.html\.erb$/)
+    end
+
     it "should pick the destdir preferrentially" do
-      expect(Tempfile).to receive(:open).with(tempname, "/foo/bar").and_return(tempfile)
+      subject = tempfile_object_for_path("/foo/bar/new_file")
+      expect(Tempfile).to receive(:open).with([tempname, ""], "/foo/bar").and_return(tempfile)
       subject.send(:tempfile_open)
     end
 
     it "should use ENV['TMP'] otherwise" do
+      subject = tempfile_object_for_path("/foo/bar/new_file")
       expect(Dir).to receive(:tmpdir).and_return("/tmp/dir")
-      expect(Tempfile).to receive(:open).with(tempname, "/foo/bar").and_raise(SystemCallError, "foo")
-      expect(Tempfile).to receive(:open).with(tempname, "/tmp/dir").and_return(tempfile)
+      expect(Tempfile).to receive(:open).with([tempname, ""], "/foo/bar").and_raise(SystemCallError, "foo")
+      expect(Tempfile).to receive(:open).with([tempname, ""], "/tmp/dir").and_return(tempfile)
       subject.send(:tempfile_open)
     end
   end