validate define concat params + split warn param

In addition, the warn param has been split into a warn (a bool) and
warn_message (a string).

.fixtures.yml

@@ -1,3 +1,7 @@
 fixtures:
+  repositories:
+    'stdlib':
+      repo: 'git://github.com/puppetlabs/puppetlabs-stdlib.git'
+      ref: '3.0.0'
   symlinks:
     'concat': '#{source_dir}'

Modulefile

@@ -6,3 +6,4 @@ license 'Apache 2.0'
 summary 'Concat module'
 description 'Concat module'
 project_page 'http://github.com/puppetlabs/puppetlabs-concat'
+dependency 'puppetlabs/stdlib', '>= 3.0.0'

manifests/init.pp

@@ -10,22 +10,27 @@
 #   The path to the final file. Use this in case you want to differentiate
 #   between the name of a resource and the file path.  Note: Use the name you
 #   provided in the target of your fragments.
-# [*mode*]
-#   The mode of the final file
 # [*owner*]
 #   Who will own the file
 # [*group*]
 #   Who will own the file
+# [*mode*]
+#   The mode of the final file
 # [*force*]
 #   Enables creating empty files if no fragments are present
 # [*warn*]
 #   Adds a normal shell style comment top of the file indicating that it is
 #   built by puppet
+# [*warn_message*]
+#   A custom message string that overides the default.
+# [*force*]
 # [*backup*]
 #   Controls the filebucketing behavior of the final file and see File type
 #   reference for its use.  Defaults to 'puppet'
 # [*replace*]
 #   Whether to replace a file that already exists on the local system
+# [*order*]
+# [*ensure_newline*]
 #
 # === Actions:
 # * Creates fragment directories if it didn't exist already
@@ -54,12 +59,26 @@ define concat(
   $group          = undef,
   $mode           = '0644',
   $warn           = false,
+  $warn_message   = undef,
   $force          = false,
   $backup         = 'puppet',
   $replace        = true,
   $order          = 'alpha',
   $ensure_newline = false,
 ) {
+  validate_re($ensure, '^present$|^absent$')
+  validate_absolute_path($path)
+  validate_string($owner)
+  validate_string($group)
+  validate_string($mode)
+  validate_bool($warn)
+  validate_string($warn_message)
+  validate_bool($force)
+  validate_string($backup)
+  validate_bool($replace)
+  validate_re($order, '^alpha$|^numeric$')
+  validate_bool($ensure_newline)
+
   include concat::setup
 
   $safe_name            = regsubst($name, '/', '_', 'G')
@@ -74,62 +93,38 @@ define concat(
     default => $group,
   }
 
-  case $warn {
+  if $warn == true {
-    'true', true, 'yes', 'on': {
+    $use_warn_message = $warn_message ? {
-      $warnmsg = $default_warn_message
+      undef   => $default_warn_message,
-    }
+      default => $warn_message,
-    'false', false, 'no', 'off': {
-      $warnmsg = ''
-    }
-    default: {
-      $warnmsg = $warn
     }
+  } else {
+    $use_warn_message = undef
   }
 
-  $warnmsg_escaped = regsubst($warnmsg, "'", "'\\\\''", 'G')
+  $warnmsg_escaped = regsubst($use_warn_message, "'", "'\\\\''", 'G')
   $warnflag = $warnmsg_escaped ? {
     ''      => '',
     default => "-w '${warnmsg_escaped}'"
   }
 
-  case $force {
+  $forceflag = $force ? {
-    'true', true, 'yes', 'on': {
+    true  => '-f',
-      $forceflag = '-f'
+    false => '',
-    }
-    'false', false, 'no', 'off': {
-      $forceflag = ''
-    }
-    default: {
-      fail("Improper 'force' value given to concat: ${force}")
-    }
   }
 
-  case $order {
+  $orderflag = $order ? {
-    'numeric': {
+    'numeric' => '-n',
-      $orderflag = '-n'
+    'alpha'   => '',
-    }
-    'alpha': {
-      $orderflag = ''
-    }
-    default: {
-      fail("Improper 'order' value given to concat: ${order}")
-    }
   }
 
-  case $ensure_newline {
+  $newlineflag = $ensure_newline ? {
-    'true', true, 'yes', 'on': {
+    true  => '-l',
-      $newlineflag = '-l'
+    false => '',
-    }
-    'false', false, 'no', 'off': {
-      $newlineflag = ''
-    }
-    default: {
-      fail("Improper 'ensure_newline' value given to concat: ${ensure_newline}")
-    }
   }
 
   File {
-    owner   => $::id,
+    owner   => $owner,
     group   => $safe_group,
     mode    => $mode,
     backup  => $backup,
@@ -168,23 +163,23 @@ define concat(
       ensure => present,
       path   => $path,
       alias  => "concat_${name}",
-      group  => $safe_group,
-      mode   => $mode,
-      owner  => $owner,
       source => "${fragdir}/${concat_name}",
     }
 
+    # remove extra whitespace from string interopolation to make testing easier
+    $command = strip(regsubst("${concat::setup::concatdir}/bin/concatfragments.sh -o ${fragdir}/${concat_name} -d ${fragdir} ${warnflag} ${forceflag} ${orderflag} ${newlineflag}", '\s+', ' ', 'G'))
+
     exec { "concat_${name}":
-      alias   => "concat_${fragdir}",
+      alias     => "concat_${fragdir}",
-      command => "${concat::setup::concatdir}/bin/concatfragments.sh -o ${fragdir}/${concat_name} -d ${fragdir} ${warnflag} ${forceflag} ${orderflag} ${newlineflag}",
+      command   => $command,
-      notify  => File[$name],
+      notify    => File[$name],
-      require => [
+      subscribe => File[$fragdir],
+      unless    => "${command} -t",
+      require   => [
         File[$fragdir],
         File["${fragdir}/fragments"],
         File["${fragdir}/fragments.concat"],
       ],
-      subscribe => File[$fragdir],
-      unless    => "${concat::setup::concatdir}/bin/concatfragments.sh -o ${fragdir}/${concat_name} -d ${fragdir} -t ${warnflag} ${forceflag} ${orderflag} ${newlineflag}",
     }
 
     if $::id == 'root' {

spec/defines/init_spec.rb

@@ -1,131 +1,381 @@
 require 'spec_helper'
 
-describe 'concat' do
+describe 'concat', :type => :define do
-  basedir = '/var/lib/puppet/concat'
-  let(:title) { '/etc/foo.bar' }
-  let(:facts) { {
-    :concat_basedir => '/var/lib/puppet/concat',
-    :id             => 'root',
-  } }
 
-  directories = [
+  shared_examples 'concat' do |title, params={}| 
-    "#{basedir}/_etc_foo.bar",
+    id = 'root'
-    "#{basedir}/_etc_foo.bar/fragments",
-  ]
 
-  directories.each do |dirs|
+    # default param values
-    it do
+    p = {
-      should contain_file(dirs).with({
+      :ensure         => 'present',
-        'ensure'  => 'directory',
+      :path           => title,
-        'backup'  => 'puppet',
+      :owner          => id,
-        'group'   => 0,
+      :group          => '0',
-        'mode'    => '0644',
+      :mode           => '0644',
-        'owner'   => 'root',
+      :warn           => false,
-      })
+      :warn_message   => nil,
+      :force          => false,
+      :backup         => 'puppet',
+      :replace        => true,
+      :order          => 'alpha',
+      :ensure_newline => false,
+    }.merge(params)
+
+    safe_name            = title.gsub('/', '_')
+    concatdir            = '/var/lib/puppet/concat'
+    fragdir              = "#{concatdir}/#{safe_name}"
+    concat_name          = 'fragments.concat.out'
+    default_warn_message = '# This file is managed by Puppet. DO NOT EDIT.'
+
+    file_defaults = {
+      :owner   => p[:owner],
+      :group   => p[:group],
+      :mode    => p[:mode],
+      :backup  => p[:backup],
+      :replace => p[:replace],
+    }
+
+    let(:title) { title }
+    let(:params) { params }
+    let(:facts) do
+      {
+        :concat_basedir => concatdir,
+        :id             => id,
+      }
+    end
+
+    if p[:ensure] == 'present'
+      it do
+        should contain_file(fragdir).with(file_defaults.merge({
+          :ensure => 'directory',
+        }))
+      end
+
+      it do
+        should contain_file("#{fragdir}/fragments").with(file_defaults.merge({
+          :ensure  => 'directory',
+          :force   => true,
+          :ignore  => ['.svn', '.git', '.gitignore'],
+          :purge   => true,
+          :recurse => true,
+          :source  => nil, # true for all but for puppet 24
+        }))
+      end
+
+      [
+        "#{fragdir}/fragments.concat",
+        "#{fragdir}/#{concat_name}",
+      ].each do |file|
+        it do
+          should contain_file(file).with(file_defaults.merge({
+            :ensure => 'present',
+          }))
+        end
+      end
+
+      it do
+        should contain_file(title).with(file_defaults.merge({
+          :ensure => 'present',
+          :path   => p[:path],
+          :alias  => "concat_#{title}",
+          :source => "#{fragdir}/#{concat_name}",
+        }))
+      end
+
+      cmd = "#{concatdir}/bin/concatfragments.sh " +
+            "-o #{concatdir}/#{safe_name}/fragments.concat.out " +
+            "-d #{concatdir}/#{safe_name}"
+
+      # flag order: fragdir, warnflag, forceflag, orderflag, newlineflag 
+      if p[:warn]
+        message = p[:warn_message] || default_warn_message
+        cmd += " -w \'#{message}\'"
+      end
+
+      cmd += " -f" if p[:force]
+      cmd += " -n" if p[:order] == 'numeric'
+      cmd += " -l" if p[:ensure_newline] == true
+
+      it do
+        should contain_exec("concat_#{title}").with({
+          :alias   => "concat_#{fragdir}",
+          :command => cmd,
+          :unless  => "#{cmd} -t",
+        })
+
+        if id == 'root'
+          should contain_exec("concat_#{title}").with({
+            :user  => 'root',
+            :group => p[:group], 
+          })
+        end
+      end
+    else
+      [
+        fragdir,
+        "#{fragdir}/fragments",
+        "#{fragdir}/fragments.concat",
+        "#{fragdir}/#{concat_name}",
+      ].each do |file|
+        it do
+          should contain_file(file).with(file_defaults.merge({
+            :ensure => 'absent',
+            :backup => false,
+            :force  => true,
+          }))
+        end
+      end
+
+      it do
+        should contain_file(title).with(file_defaults.merge({
+          :ensure => 'absent',
+        }))
+      end
+
+      it do
+        should contain_file(title).with(file_defaults.merge({
+          :ensure => 'absent',
+        }))
+      end
+
+      it do
+        should contain_exec("concat_#{title}").with({
+          :alias   => "concat_#{fragdir}",
+          :command => 'true',
+          :path    => '/bin:/usr/bin',
+        })
+      end
     end
   end
 
-  files = [
+  context 'title' do
-    "/etc/foo.bar",
+    context 'without path param' do
-    "#{basedir}/_etc_foo.bar/fragments.concat",
+      # title/name is the default value for the path param. therefore, the
-  ]
+      # title must be an absolute path unless path is specified
+      ['/foo', '/foo/bar', '/foo/bar/baz'].each do |title|
+        context title do
+          it_behaves_like 'concat', '/etc/foo.bar'
+        end
+      end
 
-  files.each do |file|
+      ['./foo', 'foo', 'foo/bar'].each do |title|
-    it do
+        context title do
-      should contain_file(file).with({
+          let(:title) { title }
-        'ensure'  => 'present',
+          it 'should fail' do
-        'backup'  => 'puppet',
+            expect { should }.to raise_error(Puppet::Error, /is not an absolute path/)
-        'group'   => 0,
+          end
-        'mode'    => '0644',
+        end
-        'owner'   => 'root',
+      end
-      })
     end
-  end
 
-  it do
+    context 'with path param' do
-    should contain_exec("concat_/etc/foo.bar").with_command(
+      ['./foo', 'foo', 'foo/bar'].each do |title|
-      "#{basedir}/bin/concatfragments.sh " +
+        context title do
-      "-o #{basedir}/_etc_foo.bar/fragments.concat.out " +
+          it_behaves_like 'concat', title, { :path => '/etc/foo.bar' }
-      "-d #{basedir}/_etc_foo.bar    "
+        end
-    )
+      end
-  end
-end
-
-describe 'concat' do
-
-  basedir = '/var/lib/puppet/concat'
-  let(:title) { 'foobar' }
-  let(:target) { '/etc/foo.bar' }
-  let(:facts) { {
-    :concat_basedir => '/var/lib/puppet/concat',
-    :id             => 'root',
-  } }
-
-  directories = [
-    "#{basedir}/foobar",
-    "#{basedir}/foobar/fragments",
-  ]
-
-  directories.each do |dirs|
-    it do
-      should contain_file(dirs).with({
-        'ensure'  => 'directory',
-        'backup'  => 'puppet',
-        'group'   => 0,
-        'mode'    => '0644',
-        'owner'   => 'root',
-      })
     end
-  end
+  end # title =>
 
-  files = [
+  context 'ensure =>' do
-    "foobar",
+    ['present', 'absent'].each do |ens|
-    "#{basedir}/foobar/fragments.concat",
+      context ens do
-  ]
+        it_behaves_like 'concat', '/etc/foo.bar', { :ensure => ens }
-
+      end
-  files.each do |file|
-    it do
-      should contain_file(file).with({
-        'ensure'  => 'present',
-        'backup'  => 'puppet',
-        'group'   => 0,
-        'mode'    => '0644',
-        'owner'   => 'root',
-      })
     end
-  end
 
-  it do
+    context 'invalid' do
-    should contain_exec("concat_foobar").with_command(
+      let(:title) { '/etc/foo.bar' }
-      "#{basedir}/bin/concatfragments.sh " +
+      let(:params) {{ :ensure => 'invalid' }}
-      "-o #{basedir}/foobar/fragments.concat.out " +
+      it 'should fail' do
-      "-d #{basedir}/foobar    "
+        expect { should }.to raise_error(Puppet::Error, /#{Regexp.escape('does not match "^present$|^absent$"')}/)
-    )
+      end
-  end
+    end
+  end # ensure =>
 
+  context 'path =>' do
+    context '/foo' do
+      it_behaves_like 'concat', '/etc/foo.bar', { :path => '/foo' }
+    end
 
-end
+    ['./foo', 'foo', 'foo/bar', false].each do |path|
+      context path do
+        let(:title) { '/etc/foo.bar' }
+        let(:params) {{ :path => path }}
+        it 'should fail' do
+          expect { should }.to raise_error(Puppet::Error, /is not an absolute path/)
+        end
+      end
+    end
+  end # path =>
 
-describe 'concat' do
+  context 'owner =>' do
-  let(:title) { '/etc/foo.bar' }
+    context 'apenney' do
-  let(:params) { {
+      it_behaves_like 'concat', '/etc/foo.bar', { :owner => 'apenny' }
-    :group => 'something', 
+    end
-    :owner => 'someone', 
+
-    :mode  => '0755' 
+    context 'false' do
-  } }
+      let(:title) { '/etc/foo.bar' }
-  let(:facts) { {
+      let(:params) {{ :owner => false }}
-    :concat_basedir => '/var/lib/puppet/concat',
+      it 'should fail' do
-    :id             => 'root',
+        expect { should }.to raise_error(Puppet::Error, /is not a string/)
-  } }
+      end
+    end
+  end # owner =>
+
+  context 'group =>' do
+    context 'apenney' do
+      it_behaves_like 'concat', '/etc/foo.bar', { :group => 'apenny' }
+    end
+
+    context 'false' do
+      let(:title) { '/etc/foo.bar' }
+      let(:params) {{ :group => false }}
+      it 'should fail' do
+        expect { should }.to raise_error(Puppet::Error, /is not a string/)
+      end
+    end
+  end # group =>
+
+  context 'mode =>' do
+    context '1755' do
+      it_behaves_like 'concat', '/etc/foo.bar', { :mode => '1755' }
+    end
+
+    context 'false' do
+      let(:title) { '/etc/foo.bar' }
+      let(:params) {{ :mode => false }}
+      it 'should fail' do
+        expect { should }.to raise_error(Puppet::Error, /is not a string/)
+      end
+    end
+  end # mode =>
+
+  context 'warn =>' do
+    [true, false].each do |warn|
+      context warn do
+        it_behaves_like 'concat', '/etc/foo.bar', { :warn => warn }
+      end
+    end
+
+    context '123' do
+      let(:title) { '/etc/foo.bar' }
+      let(:params) {{ :warn => 123 }}
+      it 'should fail' do
+        expect { should }.to raise_error(Puppet::Error, /is not a boolean/)
+      end
+    end
+  end # warn =>
+
+  context 'warn_message =>' do
+    context '# ashp replaced your file' do
+      # should do nothing unless warn == true;
+      # but we can't presently test that because concatfragments.sh isn't run
+      # from rspec-puppet tests
+      context 'warn =>' do
+        context 'true' do
+          it_behaves_like 'concat', '/etc/foo.bar', {
+            :warn         => true,
+            :warn_message => '# ashp replaced your file'
+          }
+        end
+
+        context 'false' do
+          it_behaves_like 'concat', '/etc/foo.bar', {
+            :warn         => false,
+            :warn_message => '# ashp replaced your file'
+          }
+        end
+      end
+    end
+
+    context 'false' do
+      let(:title) { '/etc/foo.bar' }
+      let(:params) {{ :warn_message => false }}
+      it 'should fail' do
+        expect { should }.to raise_error(Puppet::Error, /is not a string/)
+      end
+    end
+  end # warn_message =>
+
+  context 'force =>' do
+    [true, false].each do |force|
+      context force do
+        it_behaves_like 'concat', '/etc/foo.bar', { :force => force }
+      end
+    end
+
+    context '123' do
+      let(:title) { '/etc/foo.bar' }
+      let(:params) {{ :force => 123 }}
+      it 'should fail' do
+        expect { should }.to raise_error(Puppet::Error, /is not a boolean/)
+      end
+    end
+  end # force =>
+
+  context 'backup =>' do
+    context 'reverse' do
+      it_behaves_like 'concat', '/etc/foo.bar', { :backup => 'reverse' }
+    end
+
+    context 'false' do
+      let(:title) { '/etc/foo.bar' }
+      let(:params) {{ :backup => false }}
+      it 'should fail' do
+        expect { should }.to raise_error(Puppet::Error, /is not a string/)
+      end
+    end
+  end # backup =>
+
+  context 'replace =>' do
+    [true, false].each do |replace|
+      context replace do
+        it_behaves_like 'concat', '/etc/foo.bar', { :replace => replace }
+      end
+    end
+
+    context '123' do
+      let(:title) { '/etc/foo.bar' }
+      let(:params) {{ :replace => 123 }}
+      it 'should fail' do
+        expect { should }.to raise_error(Puppet::Error, /is not a boolean/)
+      end
+    end
+  end # replace =>
+
+  context 'order =>' do
+    ['alpha', 'numeric'].each do |order|
+      context order do
+        it_behaves_like 'concat', '/etc/foo.bar', { :order => order }
+      end
+    end
+
+    context 'invalid' do
+      let(:title) { '/etc/foo.bar' }
+      let(:params) {{ :order => 'invalid' }}
+      it 'should fail' do
+        expect { should }.to raise_error(Puppet::Error, /#{Regexp.escape('does not match "^alpha$|^numeric$"')}/)
+      end
+    end
+  end # order =>
+
+  context 'ensure_newline =>' do
+    [true, false].each do |ensure_newline|
+      context 'true' do
+        it_behaves_like 'concat', '/etc/foo.bar', { :ensure_newline => ensure_newline}
+      end
+    end
+
+    context '123' do
+      let(:title) { '/etc/foo.bar' }
+      let(:params) {{ :ensure_newline => 123 }}
+      it 'should fail' do
+        expect { should }.to raise_error(Puppet::Error, /is not a boolean/)
+      end
+    end
+  end # ensure_newline =>
 
-  it do
-    should contain_file("/etc/foo.bar").with( {
-      'ensure' => 'present',
-      'owner'  => 'someone',
-      'group'  => 'something',
-      'mode'   => '0755',
-    } )
-  end
 end
 
 # vim:sw=2:ts=2:expandtab:textwidth=79