the current code in manifests/directive.pp does :

   if versioncmp($::sudoversion,'1.7.2') >= 0 {
         file {"/etc/sudoers.d/${dname}":
              [...]
                    require => Package['sudo'],
         }
     } else {
    fail 'sudo fragments via #includedir is only available since version 1.7.2!'
  }

I think that :

• the require is not really needed as is versioncmp succeed, sudo is actually installed
• the fail should check if sudo package is missing, and issue a different error message.

The best solution would be to force package to be installed before comparison, but it's pretty difficult as facts are collected at early state.
So, I'd say a solution "better than now" would be :

if versioncmp() {
     do the stuff
} else {
     if sudo not installed {
         install it
         fail with "Sudo has just been installed, please restart puppet"
     } else {
        fail with 'sudo fragments via #includedir is only available since version 1.7.2!'
     }
}

Not sure how we can easily do this with puppet, maybe just make Package['sudo'] -> Class['sudo'] so that we ensure sudo package is installed before versioncmp is called ?

Anyway, such a solution will need 2 puppet run, but at least make it converge to a stable/working state, whereas the current code break runs unless you install sudo by hand.

Maxence

I was unable to apply the module as is. It returned the following error:

err: Failed to apply catalog: 'visudo -c -f /etc/sudoers.d/foo || ( rm -f /etc/sudoers.d/foo && exit 1)' is not qualified and no path was specified. Please qualify the command or specify a path.

A simple path qualification corrected this error.

diff --git a/manifests/directive.pp b/manifests/directive.pp
index 3ad5f02..e3db631 100644
--- a/manifests/directive.pp
+++ b/manifests/directive.pp
@@ -46,7 +46,7 @@ define sudo::directive (
   }

   exec {"sudo-syntax-check for file $dname":
-    command     => "visudo -c -f /etc/sudoers.d/${dname} || ( rm -f /etc/sudoers.d/${dname} && exit 1)",
+    command     => "/usr/sbin/visudo -c -f /etc/sudoers.d/${dname} || ( rm -f /etc/sudoers.d/${dname} && exit 1)",
     refreshonly => true,
   }

I am running Debian Squeeze on both puppet-master and agent.

Cheers,
Ignace M

Hi,

The sudo fact alters ENV['PATH'] and does not restore it, which breaks other facts or providers requiring binaries located in previously defined PATH entries (like /usr/local/bin for example).

I don't know the rationale to change PATH at this place (found a puppetlabs ticket that might require the workaround in place), but should'nt it be at least restored at the end of the function ? Or just check for missing entries and append them to the PATH ?

Cheers.