Merge pull request #95 from jhoblitt/fragment_params

deprecate concat::fragment mode, owner, & group params

manifests/fragment.pp

@@ -16,11 +16,11 @@
 # [*ensure*]
 #   Present/Absent or destination to a file to include another file
 # [*mode*]
-#   Mode for the file
+#   Deprecated
 # [*owner*]
-#   Owner of the file
+#   Deprecated
 # [*group*]
-#   Owner of the file
+#   Deprecated
 # [*backup*]
 #   Deprecated
 #
@@ -30,7 +30,7 @@ define concat::fragment(
     $source  = undef,
     $order   = 10,
     $ensure  = 'present',
-    $mode    = '0640',
+    $mode    = undef,
     $owner   = undef,
     $group   = undef,
     $backup  = undef
@@ -40,9 +40,15 @@ define concat::fragment(
   validate_string($content)
   validate_string($source)
   validate_string($order)
-  validate_string($mode)
+  if $mode {
-  validate_string($owner)
+    warning('The $mode parameter to concat::fragment is deprecated and has no effect')
-  validate_string($group)
+  }
+  if $owner {
+    warning('The $owner parameter to concat::fragment is deprecated and has no effect')
+  }
+  if $group {
+    warning('The $group parameter to concat::fragment is deprecated and has no effect')
+  }
   if $backup {
     warning('The $backup parameter to concat::fragment is deprecated and has no effect')
   }
@@ -67,11 +73,12 @@ define concat::fragment(
     }
   }
 
+  # 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,
-    mode    => $mode,
+    owner   => $::id,
-    owner   => $owner,
+    mode    => '0640',
-    group   => $group,
     source  => $source,
     content => $content,
     backup  => false,

spec/system/deprecation_warnings_spec.rb

@@ -26,6 +26,45 @@ describe 'deprecation warnings' do
     it_behaves_like 'has_warning', pp, w
   end
 
+  context 'concat::fragment mode parameter' do
+    pp="
+      concat { '/tmp/file': }
+      concat::fragment { 'foo':
+        target => '/tmp/file',
+        mode   => 'bar',
+      }
+    "
+    w = 'The $mode parameter to concat::fragment is deprecated and has no effect'
+
+    it_behaves_like 'has_warning', pp, w
+  end
+
+  context 'concat::fragment owner parameter' do
+    pp="
+      concat { '/tmp/file': }
+      concat::fragment { 'foo':
+        target => '/tmp/file',
+        owner  => 'bar',
+      }
+    "
+    w = 'The $owner parameter to concat::fragment is deprecated and has no effect'
+
+    it_behaves_like 'has_warning', pp, w
+  end
+
+  context 'concat::fragment group parameter' do
+    pp="
+      concat { '/tmp/file': }
+      concat::fragment { 'foo':
+        target => '/tmp/file',
+        group  => 'bar',
+      }
+    "
+    w = 'The $group parameter to concat::fragment is deprecated and has no effect'
+
+    it_behaves_like 'has_warning', pp, w
+  end
+
   context 'concat::fragment backup parameter' do
     pp="
       concat { '/tmp/file': }

spec/unit/defines/concat_fragment_spec.rb

@@ -10,19 +10,16 @@ describe 'concat::fragment', :type => :define do
       :source  => nil,
       :order   => 10,
       :ensure  => 'present',
-      :mode    => '0640',
-      :owner   => nil,
-      :group   => nil,
-      :backup  => 'puppet',
     }.merge(params)
 
     safe_name        = title.gsub(/[\/\n]/, '_')
     safe_target_name = p[:target].gsub(/[\/\n]/, '_')
     concatdir        = '/var/lib/puppet/concat'
     fragdir          = "#{concatdir}/#{safe_target_name}"
+    id               = 'root'
 
     let(:title) { title }
-    let(:facts) {{ :concat_basedir => concatdir }}
+    let(:facts) {{ :concat_basedir => concatdir, :id => id }}
     let(:params) { params }
     let(:pre_condition) do
       "concat{ '#{p[:target]}': }"
@@ -33,9 +30,8 @@ describe 'concat::fragment', :type => :define do
       should contain_concat(p[:target])
       should contain_file("#{fragdir}/fragments/#{p[:order]}_#{safe_name}").with({
         :ensure  => p[:ensure],
-        :mode    => p[:mode],
+        :owner   => id,
-        :owner   => p[:owner],
+        :mode    => '0640',
-        :group   => p[:group],
         :source  => p[:source],
         :content => p[:content],
         :alias   => "concat_fragment_#{title}",
@@ -156,21 +152,16 @@ describe 'concat::fragment', :type => :define do
     end
   end # order =>
 
+  describe 'deprecated parameter' do
     context 'mode =>' do
       context '1755' do
         it_behaves_like 'fragment', 'motd_header', {
           :mode   => '1755',
           :target => '/etc/motd',
         }
-    end
 
-    context 'false' do
+        it 'should create a warning' do
-      let(:title) { 'motd_header' }
+          pending('rspec-puppet support for testing warning()')
-      let(:facts) {{ :concat_basedir => '/tmp' }}
-      let(:params) {{ :mode => false, :target => '/etc/motd' }}
-
-      it 'should fail' do
-        expect { should }.to raise_error(Puppet::Error, /is not a string/)
         end
       end
     end # mode =>
@@ -181,15 +172,9 @@ describe 'concat::fragment', :type => :define do
           :owner  => 'apenny',
           :target => '/etc/motd',
         }
-    end
 
-    context 'false' do
+        it 'should create a warning' do
-      let(:title) { 'motd_header' }
+          pending('rspec-puppet support for testing warning()')
-      let(:facts) {{ :concat_basedir => '/tmp' }}
-      let(:params) {{ :owner => false, :target => '/etc/motd' }}
-
-      it 'should fail' do
-        expect { should }.to raise_error(Puppet::Error, /is not a string/)
         end
       end
     end # owner =>
@@ -200,28 +185,25 @@ describe 'concat::fragment', :type => :define do
           :group  => 'apenny',
           :target => '/etc/motd',
         }
-    end
 
-    context 'false' do
+        it 'should create a warning' do
-      let(:title) { 'motd_header' }
+          pending('rspec-puppet support for testing warning()')
-      let(:facts) {{ :concat_basedir => '/tmp' }}
-      let(:params) {{ :group => false, :target => '/etc/motd' }}
-
-      it 'should fail' do
-        expect { should }.to raise_error(Puppet::Error, /is not a string/)
         end
       end
     end # group =>
 
-  # should raise a warning but rspec-puppet can't presently test for warning()
-  # we can only test for the existence of the parameter
     context 'backup =>' do
       context 'foo' do
         it_behaves_like 'fragment', 'motd_header', {
           :backup => 'foo',
           :target => '/etc/motd',
         }
+
+        it 'should create a warning' do
+          pending('rspec-puppet support for testing warning()')
+        end
       end
     end # backup =>
+  end # deprecated params
 
 end

spec/unit/defines/concat_spec.rb

@@ -367,11 +367,15 @@ describe 'concat', :type => :define do
     end
   end # ensure_newline =>
 
-  # should raise a warning but rspec-puppet can't presently test for warning()
+  describe 'deprecated parameter' do
-  # we can only test for the existence of the parameter
     context 'gnu =>' do
       context 'foo' do
         it_behaves_like 'concat', '/etc/foo.bar', { :gnu => 'foo'}
+
+        it 'should create a warning' do
+          pending('rspec-puppet support for testing warning()')
+        end
+      end
     end
   end