fix regression preventing usage of fragment ensure => /target syntax

Also fix a historical bug that prevents a fragment from being converted
from a symlink to using a source or content parameter.

manifests/fragment.pp

@@ -36,7 +36,9 @@ define concat::fragment(
     $backup  = undef
 ) {
   validate_string($target)
-  validate_re($ensure, '^$|^present$|^absent$|^file$|^directory$')
+  if ! ($ensure in [ 'present', 'absent' ]) {
+    warning('Passing a value other than \'present\' or \'absent\' as the $ensure parameter to concat::fragment is deprecated.  If you want to use the content of a file as a fragment please use the $source parameter.')
+  }
   validate_string($content)
   if !(is_string($source) or is_array($source)) {
     fail('$source is not a string or an Array.')
@@ -62,23 +64,43 @@ define concat::fragment(
   $concatdir        = $concat::setup::concatdir
   $fragdir          = "${concatdir}/${safe_target_name}"
 
-  # if content is passed, use that, else if source is passed use that
+  # The file type's semantics are problematic in that ensure => present will
-  # if neither passed, but $ensure is in symlink form, make a symlink
+  # not over write a pre-existing symlink.  We are attempting to provide
-  case $ensure {
+  # backwards compatiblity with previous concat::fragment versions that
-    '', 'absent', 'present', 'file', 'directory': {
+  # supported the file type's ensure => /target syntax
-      if ! ($content or $source) {
+
-        crit('No content, source or symlink specified')
+  # be paranoid and only allow the fragment's file resource's ensure param to
-      }
+  # be file, absent, or a file target
-    }
+  $safe_ensure = $ensure ? {
-    default: {
+    ''        => 'file',
-      # do nothing, make puppet-lint happy
+    undef     => 'file',
-    }
+    'file'    => 'file',
+    'present' => 'file',
+    'absent'  => 'absent',
+    default   => $ensure,
+  }
+
+  # if it looks line ensure => /target syntax was used, fish that out
+  if ! ($ensure in ['', 'present', 'absent', 'file' ]) {
+    $ensure_target = $ensure
+  }
+
+  # the file type's semantics only allows one of: ensure => /target, content,
+  # or source
+  if ($ensure_target and $source) or
+    ($ensure_target and $content) or
+    ($source and $content) {
+    fail('You cannot specify more than one of $content, $source, $ensure => /target')
+  }
+
+  if ! ($content or $source or $ensure_target) {
+    crit('No content, source or symlink specified')
   }
 
   # punt on group ownership until some point in the distant future when $::gid
   # can be relied on to be present
   file { "${fragdir}/fragments/${order}_${safe_name}":
-    ensure  => $ensure,
+    ensure  => $safe_ensure,
     owner   => $::id,
     mode    => '0640',
     source  => $source,

spec/system/deprecation_warnings_spec.rb

@@ -18,7 +18,8 @@ describe 'deprecation warnings' do
         gnu => 'foo',
       }
       concat::fragment { 'foo':
-        target => '/tmp/concat/file',
+        target  => '/tmp/concat/file',
+        content => 'bar',
       }
     EOS
     w = 'The $gnu parameter to concat is deprecated and has no effect'
@@ -26,12 +27,117 @@ describe 'deprecation warnings' do
     it_behaves_like 'has_warning', pp, w
   end
 
+  context 'concat::fragment ensure parameter' do
+    context 'target file exists' do
+      before(:all) do
+        shell("/bin/echo 'file1 contents' > /tmp/concat/file1")
+      end
+      after(:all) do
+        # XXX this test may leave behind a symlink in the fragment directory
+        # which could cause warnings and/or breakage from the subsequent tests
+        # unless we clean it up. 
+        shell('rm -rf /tmp/concat /var/lib/puppet/concat')
+        shell('mkdir -p /tmp/concat')
+      end
+
+      pp = <<-EOS
+        concat { '/tmp/concat/file': }
+        concat::fragment { 'foo':
+          target => '/tmp/concat/file',
+          ensure => '/tmp/concat/file1',
+        }
+      EOS
+      w = 'Passing a value other than \'present\' or \'absent\' as the $ensure parameter to concat::fragment is deprecated.  If you want to use the content of a file as a fragment please use the $source parameter.'
+
+      it_behaves_like 'has_warning', pp, w
+
+      describe file('/tmp/concat/file') do
+        it { should be_file }
+        it { should contain 'file1 contents' }
+      end
+
+      # check that the fragment can be changed from a symlink to a plain file
+
+      pp = <<-EOS
+        concat { '/tmp/concat/file': }
+        concat::fragment { 'foo':
+          target  => '/tmp/concat/file',
+          content => 'new content',
+        }
+      EOS
+
+      context puppet_apply(pp) do
+        its(:stderr) { should be_empty }
+        its(:exit_code) { should_not == 1 }
+        its(:refresh) { should be_nil }
+        its(:stderr) { should be_empty }
+        its(:exit_code) { should be_zero }
+      end
+
+      describe file('/tmp/concat/file') do
+        it { should be_file }
+        it { should contain 'new content' }
+        it { should_not contain 'file1 contents' }
+      end
+    end # target file exists
+
+    context 'target does not exist' do
+      after(:all) do
+        # XXX this test may leave behind a symlink in the fragment directory
+        # which could cause warnings and/or breakage from the subsequent tests
+        # unless we clean it up. 
+        shell('rm -rf /tmp/concat /var/lib/puppet/concat')
+        shell('mkdir -p /tmp/concat')
+      end
+
+      pp = <<-EOS
+        concat { '/tmp/concat/file': }
+        concat::fragment { 'foo':
+          target => '/tmp/concat/file',
+          ensure => '/tmp/concat/file1',
+        }
+      EOS
+      w = 'Passing a value other than \'present\' or \'absent\' as the $ensure parameter to concat::fragment is deprecated.  If you want to use the content of a file as a fragment please use the $source parameter.'
+
+      it_behaves_like 'has_warning', pp, w
+
+      describe file('/tmp/concat/file') do
+        it { should be_file }
+      end
+
+      # check that the fragment can be changed from a symlink to a plain file
+
+      pp = <<-EOS
+        concat { '/tmp/concat/file': }
+        concat::fragment { 'foo':
+          target  => '/tmp/concat/file',
+          content => 'new content',
+        }
+      EOS
+
+      context puppet_apply(pp) do
+        its(:stderr) { should be_empty }
+        its(:exit_code) { should_not == 1 }
+        its(:refresh) { should be_nil }
+        its(:stderr) { should be_empty }
+        its(:exit_code) { should be_zero }
+      end
+
+      describe file('/tmp/concat/file') do
+        it { should be_file }
+        it { should contain 'new content' }
+      end
+    end # target file exists
+
+  end # concat::fragment ensure parameter
+
   context 'concat::fragment mode parameter' do
     pp = <<-EOS
       concat { '/tmp/concat/file': }
       concat::fragment { 'foo':
-        target => '/tmp/concat/file',
+        target  => '/tmp/concat/file',
-        mode   => 'bar',
+        content => 'bar',
+        mode    => 'bar',
       }
     EOS
     w = 'The $mode parameter to concat::fragment is deprecated and has no effect'
@@ -43,8 +149,9 @@ describe 'deprecation warnings' do
     pp = <<-EOS
       concat { '/tmp/concat/file': }
       concat::fragment { 'foo':
-        target => '/tmp/concat/file',
+        target  => '/tmp/concat/file',
-        owner  => 'bar',
+        content => 'bar',
+        owner   => 'bar',
       }
     EOS
     w = 'The $owner parameter to concat::fragment is deprecated and has no effect'
@@ -56,8 +163,9 @@ describe 'deprecation warnings' do
     pp = <<-EOS
       concat { '/tmp/concat/file': }
       concat::fragment { 'foo':
-        target => '/tmp/concat/file',
+        target  => '/tmp/concat/file',
-        group  => 'bar',
+        content => 'bar',
+        group   => 'bar',
       }
     EOS
     w = 'The $group parameter to concat::fragment is deprecated and has no effect'
@@ -69,8 +177,9 @@ describe 'deprecation warnings' do
     pp = <<-EOS
       concat { '/tmp/concat/file': }
       concat::fragment { 'foo':
-        target => '/tmp/concat/file',
+        target  => '/tmp/concat/file',
-        backup => 'bar',
+        content => 'bar',
+        backup  => 'bar',
       }
     EOS
     w = 'The $backup parameter to concat::fragment is deprecated and has no effect'

spec/unit/defines/concat_fragment_spec.rb

@@ -17,6 +17,11 @@ describe 'concat::fragment', :type => :define do
     concatdir        = '/var/lib/puppet/concat'
     fragdir          = "#{concatdir}/#{safe_target_name}"
     id               = 'root'
+    if p[:ensure] == 'absent'
+      safe_ensure = p[:ensure] 
+    else
+      safe_ensure = 'file'
+    end
 
     let(:title) { title }
     let(:facts) {{ :concat_basedir => concatdir, :id => id }}
@@ -29,7 +34,7 @@ describe 'concat::fragment', :type => :define do
       should contain_class('concat::setup')
       should contain_concat(p[:target])
       should contain_file("#{fragdir}/fragments/#{p[:order]}_#{safe_name}").with({
-        :ensure  => p[:ensure],
+        :ensure  => safe_ensure,
         :owner   => id,
         :mode    => '0640',
         :source  => p[:source],
@@ -69,7 +74,7 @@ describe 'concat::fragment', :type => :define do
   end # target =>
 
   context 'ensure =>' do
-    ['', 'present', 'absent', 'file', 'directory'].each do |ens|
+    ['present', 'absent'].each do |ens|
       context ens do
         it_behaves_like 'fragment', 'motd_header', {
           :ensure => ens,
@@ -78,13 +83,13 @@ describe 'concat::fragment', :type => :define do
       end
     end
 
-    context 'invalid' do
+    context 'any value other than \'present\' or \'absent\'' do
       let(:title) { 'motd_header' }
       let(:facts) {{ :concat_basedir => '/tmp' }}
       let(:params) {{ :ensure => 'invalid', :target => '/etc/motd' }}
 
-      it 'should fail' do
+      it 'should create a warning' do
-        expect { should }.to raise_error(Puppet::Error, /#{Regexp.escape('does not match "^$|^present$|^absent$|^file$|^directory$"')}/)
+        pending('rspec-puppet support for testing warning()')
       end
     end
   end # ensure =>
@@ -152,6 +157,59 @@ describe 'concat::fragment', :type => :define do
     end
   end # order =>
 
+  context 'more than one content source' do
+    error_msg = 'You cannot specify more than one of $content, $source, $ensure => /target'
+
+    context 'ensure => target and source' do
+      let(:title) { 'motd_header' }
+      let(:facts) {{ :concat_basedir => '/tmp' }}
+      let(:params) do
+        {
+          :target  => '/etc/motd',
+          :ensure  => '/foo',
+          :source  => '/bar',
+        }
+      end
+
+      it 'should fail' do
+        expect { should }.to raise_error(Puppet::Error, /#{Regexp.escape(error_msg)}/m)
+      end
+    end
+
+    context 'ensure => target and content' do
+      let(:title) { 'motd_header' }
+      let(:facts) {{ :concat_basedir => '/tmp' }}
+      let(:params) do
+        {
+          :target  => '/etc/motd',
+          :ensure  => '/foo',
+          :content => 'bar',
+        }
+      end
+
+      it 'should fail' do
+        expect { should }.to raise_error(Puppet::Error, /#{Regexp.escape(error_msg)}/m)
+      end
+    end
+
+    context 'source and content' do
+      let(:title) { 'motd_header' }
+      let(:facts) {{ :concat_basedir => '/tmp' }}
+      let(:params) do
+        {
+          :target => '/etc/motd',
+          :source => '/foo',
+          :content => 'bar',
+        }
+      end
+
+      it 'should fail' do
+        expect { should }.to raise_error(Puppet::Error, /#{Regexp.escape(error_msg)}/m)
+      end
+    end
+
+  end # more than one content source
+
   describe 'deprecated parameter' do
     context 'mode =>' do
       context '1755' do