From cdb6d6b007d797f95d8263e5b67fa2e4e40e73b6 Mon Sep 17 00:00:00 2001
From: Joshua Hoblitt <jhoblitt@cpan.org>
Date: Tue, 15 Oct 2013 14:07:51 -0700
Subject: [PATCH] remove undocumented requirement to include concat::setup in
 manifest

Unless the class `concat::setup` has been manually included into the
manifest before using the `concat` / `concat::fragment` defined types,
the puppet master will generate this warning while compiling the catalog.

    Tue Oct 15 14:05:06 -0700 2013 Scope(Concat[/etc/exports]) (warning):
    Could not look up qualified variable 'concat::setup::root_group'; class
    concat::setup has not been evaluated

The need to `include concat::setup` directly into the manifest has never
been part of the documented API.
---
 README.markdown                      |  4 +++
 manifests/fragment.pp                | 14 ++++++++---
 manifests/init.pp                    | 13 +++++++---
 manifests/setup.pp                   |  3 ---
 spec/defines/concat_fragment_spec.rb | 37 ++++++++++++++++++++++++++++
 spec/defines/init_spec.rb            |  6 -----
 6 files changed, 61 insertions(+), 16 deletions(-)
 create mode 100644 spec/defines/concat_fragment_spec.rb

diff --git a/README.markdown b/README.markdown
index 9580c9d..5e950b7 100644
--- a/README.markdown
+++ b/README.markdown
@@ -149,6 +149,10 @@ Contributors:
 
  * Configurable paths
 
+**Joshua Hoblitt**
+
+ * Remove requirement to manually include `concat::setup` in the manifest
+
 Contact:
 --------
 puppet-users@ mailing list.
diff --git a/manifests/fragment.pp b/manifests/fragment.pp
index a6831f8..1d883cb 100644
--- a/manifests/fragment.pp
+++ b/manifests/fragment.pp
@@ -33,13 +33,21 @@ define concat::fragment(
     $ensure = 'present',
     $mode = '0644',
     $owner = $::id,
-    $group = $concat::setup::root_group,
-    $backup = 'puppet') {
+    $group = undef,
+    $backup = 'puppet'
+) {
+  include concat::setup
+
   $safe_name = regsubst($name, '[/\n]', '_', 'GM')
   $safe_target_name = regsubst($target, '[/\n]', '_', 'GM')
   $concatdir = $concat::setup::concatdir
   $fragdir = "${concatdir}/${safe_target_name}"
 
+  $safe_group = $group ? {
+    undef   => $concat::setup::root_group,
+    default => $group,
+  }
+
   # if content is passed, use that, else if source is passed use that
   # if neither passed, but $ensure is in symlink form, make a symlink
   case $ensure {
@@ -57,7 +65,7 @@ define concat::fragment(
     ensure  => $ensure,
     mode    => $mode,
     owner   => $owner,
-    group   => $group,
+    group   => $safe_group,
     source  => $source,
     content => $content,
     backup  => $backup,
diff --git a/manifests/init.pp b/manifests/init.pp
index d132e43..214c929 100644
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -51,7 +51,7 @@ define concat(
   $ensure = 'present',
   $path = $name,
   $owner = $::id,
-  $group = $concat::setup::root_group,
+  $group = undef,
   $mode = '0644',
   $warn = false,
   $force = false,
@@ -70,6 +70,11 @@ define concat(
   $concat_name = 'fragments.concat.out'
   $default_warn_message = '# This file is managed by Puppet. DO NOT EDIT.'
 
+  $safe_group = $group ? {
+    undef   => $concat::setup::root_group,
+    default => $safe_group,
+  }
+
   case $warn {
     'true', true, yes, on: {
       $warnmsg = $default_warn_message
@@ -126,7 +131,7 @@ define concat(
 
   File {
     owner   => $::id,
-    group   => $group,
+    group   => $safe_group,
     mode    => $mode,
     backup  => $backup,
     replace => $replace
@@ -164,7 +169,7 @@ define concat(
       ensure   => present,
       path     => $path,
       alias    => "concat_${name}",
-      group    => $group,
+      group    => $safe_group,
       mode     => $mode,
       owner    => $owner,
       source   => "${fragdir}/${concat_name}",
@@ -186,7 +191,7 @@ define concat(
     if $::id == 'root' {
       Exec["concat_${name}"] {
         user  => root,
-        group => $group,
+        group => $safe_group,
       }
     }
   }
diff --git a/manifests/setup.pp b/manifests/setup.pp
index 55d7197..4dc5129 100644
--- a/manifests/setup.pp
+++ b/manifests/setup.pp
@@ -53,7 +53,4 @@ class concat::setup {
     ensure => absent;
   }
 
-  # Ensure we run setup first.
-  Class['concat::setup'] -> Concat::Fragment<| |>
-
 }
diff --git a/spec/defines/concat_fragment_spec.rb b/spec/defines/concat_fragment_spec.rb
new file mode 100644
index 0000000..8a37247
--- /dev/null
+++ b/spec/defines/concat_fragment_spec.rb
@@ -0,0 +1,37 @@
+require 'spec_helper'
+
+describe 'concat::fragment', :type => :defne do
+  let(:title) { 'motd_header' }
+
+  let(:facts) do
+    {
+     :concat_basedir => '/var/lib/puppet/concat',
+     :id             => 'root',
+    }
+  end
+
+  let :pre_condition do
+    "concat{ '/etc/motd': }"
+  end
+
+  context 'target => /etc/motd' do
+    let(:params) {{ :target => '/etc/motd' }}
+    it do
+      should contain_class('concat::setup')
+      should contain_concat('/etc/motd')
+      should contain_concat__fragment('motd_header').with({
+        :target => '/etc/motd',
+      })
+      should contain_file('/var/lib/puppet/concat/_etc_motd/fragments/10_motd_header' ).with({
+        :ensure  => 'present',
+        :mode    => '0644',
+        :owner   => 'root',
+        :group   => 0,
+        :source  => nil,
+        :content => nil,
+        :backup  => 'puppet',
+        :alias   => 'concat_fragment_motd_header',
+      })
+    end
+  end
+end
diff --git a/spec/defines/init_spec.rb b/spec/defines/init_spec.rb
index ace50f0..4195d55 100644
--- a/spec/defines/init_spec.rb
+++ b/spec/defines/init_spec.rb
@@ -7,9 +7,6 @@ describe 'concat' do
     :concat_basedir => '/var/lib/puppet/concat',
     :id             => 'root',
   } }
-  let :pre_condition do
-    'include concat::setup'
-  end
 
   directories = [
     "#{basedir}/_etc_foo.bar",
@@ -63,9 +60,6 @@ describe 'concat' do
     :concat_basedir => '/var/lib/puppet/concat',
     :id             => 'root',
   } }
-  let :pre_condition do
-    'include concat::setup'
-  end
 
   directories = [
     "#{basedir}/foobar",