Enabling syntax checking for puppet via a git update hook

[Todd Zullinger]

I was talking with Ricky on IRC about enabling some syntax checking
for puppet *.pp files in a git update hook for the infra puppet repo.
The goal would be to help catch typos before they ever get pushed into
the main /git/puppet repository.

I've tested this a bit on puppet1 with local repos and it appears to
work well.  But obviously, we'd want to enable this when there were
plenty of folks around to revert it should it cause and prevent work
from getting done.

While testing, I noticed that the update hook in /git/puppet is pretty
much identical to the fedora-git-commit-mail-hook in use on hosted.
Perhaps we'd want to be guinea pigs for converting from that older
update hook to the more current update hook and have mail
notifications handled via the post-receive-email hook?  That would
allow us to work out any issues before we enable that on hosted.

I think we would probably want to make this change first, ensure it
doesn't cause any problems, and then add the puppet syntax checking to
the update hook.

Assuming we make the above changes and use the post-receive-email hook
for mail notification, the changes I'd like to make to the current
git-1.5.5.6 update hook to enable puppet syntax checking are:

--- /usr/share/git-core/templates/hooks/update	2008-12-20 06:10:22.000000000 +0000
+++ /home/fedora/tmz/puppet.git/hooks/update	2009-06-25 01:34:14.000000000 +0000
@@ -103,5 +103,40 @@
 		;;
 esac
 
+# Check syntax of puppet files
+# Taken from http://reductivelabs.com/trac/puppet/wiki/PuppetVersionControl
+
+check="puppet --color=false --confdir=/tmp --vardir=/tmp --parseonly
--ignoreimport"
+tmp=$(mktemp /tmp/git.update.XXXXXX)
+log=$(mktemp /tmp/git.update.log.XXXXXX)
+tree=$(mktemp /tmp/git.diff-tree.XXXXXX)
+
+git diff-tree -r "$2" "$3" > $tree
+
+exit_status=0
+
+while read old_mode new_mode old_sha1 new_sha1 status name
+do
+    # skip lines showing parent commit
+    test -z "$new_sha1" && continue
+    # Only test .pp files
+    if [[ $name =~ [.]pp$ ]]
+    then
+        git cat-file blob $new_sha1 > $tmp
+        set -o pipefail
+        $check $tmp 2>&1 | sed
"s|/tmp/git.update.*:\([0-9]*\)$|${name}:\1|" > $log
+        if [[ $? != 0 ]]
+        then
+            echo
+            cat $log >&2
+            echo -e "For details run: git diff ${old_sha1:0:7} ${new_sha1:0:7}"
>&2
+            echo
+            exit_status=1
+        fi
+    fi
+done < $tree
+
+rm -f $log $tmp $tree
+
 # --- Finished
-exit 0
+exit $exit_status

When pushing an update with a syntax errors, the output looks like
this:

$ git push ~/puppet.git test-hooks 
Counting objects: 14, done.
Compressing objects: 100% (9/9), done.
Writing objects: 100% (9/9), 767 bytes, done.
Total 9 (delta 7), reused 0 (delta 0)
Unpacking objects: 100% (9/9), done.

err: Could not parse for environment production: Syntax error at 'source';
expected '}' at manifests/filetypes/standard.pp:11
For details run: git diff 3d15e34 d71d226


err: Could not parse for environment production: Syntax error at 'group'; expected
'}' at manifests/site.pp:12
For details run: git diff 22d6265 62e516f

error: hooks/update exited with error code 1
error: hook declined to update refs/heads/test-hooks
To /home/fedora/tmz/puppet.git
 ! [remote rejected] test-hooks -> test-hooks (hook declined)
error: failed to push some refs to '/home/fedora/tmz/puppet.git'

-- 
Todd        OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Nothing is wrong with California that a rise in the ocean level
wouldn't cure.
    -- Ross MacDonald (1915-1983)

[Mike McGrath]

On Wed, 24 Jun 2009, Todd Zullinger wrote:

>  I was talking with Ricky on IRC about enabling some syntax checking
>  for puppet *.pp files in a git update hook for the infra puppet repo.
>  The goal would be to help catch typos before they ever get pushed into
>  the main /git/puppet repository.
>
>  I've tested this a bit on puppet1 with local repos and it appears to
>  work well.  But obviously, we'd want to enable this when there were
>  plenty of folks around to revert it should it cause and prevent work
>  from getting done.
>
>  While testing, I noticed that the update hook in /git/puppet is pretty
>  much identical to the fedora-git-commit-mail-hook in use on hosted.
>  Perhaps we'd want to be guinea pigs for converting from that older
>  update hook to the more current update hook and have mail
>  notifications handled via the post-receive-email hook?  That would
>  allow us to work out any issues before we enable that on hosted.
>
>  I think we would probably want to make this change first, ensure it
>  doesn't cause any problems, and then add the puppet syntax checking to
>  the update hook.
>
>  Assuming we make the above changes and use the post-receive-email hook
>  for mail notification, the changes I'd like to make to the current
>  git-1.5.5.6 update hook to enable puppet syntax checking are:
>  
I'll take a look at this tomorrow, we've got a git check in there now that
does a syntax and notify.  I think the only reason it prevents commits is
because I didn't know how to do that :)  so all it does is throw errors.

Here's the only got'cha.  We mix a private and public repo together.  IE:
in our public repo we reference $someDbPassword, and then in the private
repo we create that password.  The only time they're together is after a
push has happened.  Does this account for that?  Does that problem not
even exist anymore?

	-Mike


>  --- /usr/share/git-core/templates/hooks/update	2008-12-20
> 06:10:22.000000000 +0000
>  +++ /home/fedora/tmz/puppet.git/hooks/update	2009-06-25 01:34:14.000000000 +0000
>  @@ -103,5 +103,40 @@
>   		;;
>   esac
>
>  +# Check syntax of puppet files
>  +# Taken from http://reductivelabs.com/trac/puppet/wiki/PuppetVersionControl
>  +
>  +check="puppet --color=false --confdir=/tmp --vardir=/tmp --parseonly
> --ignoreimport"
>  +tmp=$(mktemp /tmp/git.update.XXXXXX)
>  +log=$(mktemp /tmp/git.update.log.XXXXXX)
>  +tree=$(mktemp /tmp/git.diff-tree.XXXXXX)
>  +
>  +git diff-tree -r "$2" "$3" > $tree
>  +
>  +exit_status=0
>  +
>  +while read old_mode new_mode old_sha1 new_sha1 status name
>  +do
>  +    # skip lines showing parent commit
>  +    test -z "$new_sha1" && continue
>  +    # Only test .pp files
>  +    if [[ $name =~ [.]pp$ ]]
>  +    then
>  +        git cat-file blob $new_sha1 > $tmp
>  +        set -o pipefail
>  +        $check $tmp 2>&1 | sed
> "s|/tmp/git.update.*:\([0-9]*\)$|${name}:\1|" > $log
>  +        if [[ $? != 0 ]]
>  +        then
>  +            echo
>  +            cat $log >&2
>  +            echo -e "For details run: git diff ${old_sha1:0:7}
> ${new_sha1:0:7}" >&2
>  +            echo
>  +            exit_status=1
>  +        fi
>  +    fi
>  +done < $tree
>  +
>  +rm -f $log $tmp $tree
>  +
>   # --- Finished
>  -exit 0
>  +exit $exit_status
>
>  When pushing an update with a syntax errors, the output looks like
>  this:
>
>  $ git push ~/puppet.git test-hooks
>  Counting objects: 14, done.
>  Compressing objects: 100% (9/9), done.
>  Writing objects: 100% (9/9), 767 bytes, done.
>  Total 9 (delta 7), reused 0 (delta 0)
>  Unpacking objects: 100% (9/9), done.
>
>  err: Could not parse for environment production: Syntax error at 'source';
> expected '}' at manifests/filetypes/standard.pp:11
>  For details run: git diff 3d15e34 d71d226
>
>
>  err: Could not parse for environment production: Syntax error at 'group';
> expected '}' at manifests/site.pp:12
>  For details run: git diff 22d6265 62e516f
>
>  error: hooks/update exited with error code 1
>  error: hook declined to update refs/heads/test-hooks
>  To /home/fedora/tmz/puppet.git
>   ! [remote rejected] test-hooks -> test-hooks (hook declined)
>  error: failed to push some refs to '/home/fedora/tmz/puppet.git'
>
>  --
>  Todd        OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  Nothing is wrong with California that a rise in the ocean level
>  wouldn't cure.
>      -- Ross MacDonald (1915-1983)
>
>

[Todd Zullinger]

Mike McGrath wrote:
>  I'll take a look at this tomorrow, we've got a git check in
> there
>  now that does a syntax and notify.  I think the only reason it
>  prevents commits is because I didn't know how to do that :)  so all
>  it does is throw errors. 
That's in syncPuppetMaster.sh, called from the post-update hook,
right?  By then, there is no chance to deny the push, as the refs have
been updated by git. :)

>  Here's the only got'cha.  We mix a private and public repo
> together.
>  IE: in our public repo we reference $someDbPassword, and then in the
>  private repo we create that password.  The only time they're
>  together is after a push has happened.  Does this account for that?
>  Does that problem not even exist anymore? 
Using the code for the update hook in my previous mail, I don't think
is should be a problem.  That should only check the files that are
being modified by the push for syntax errors.  Puppet is called with
--parseonly and --ignoreimport.  That should prevent problems caused
by a manifest in puppet relying on something in private.  Of course,
testing it on a manifest that uses a variable define in private would
be a good idea. :)

Keeping the syntax check in syncPuppetMaster.sh is probably a good
backup, as it might catch things that the check on individual .pp
files misses.

-- 
Todd        OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
You're not drunk if you can lie on the floor without holding on.
    -- Dean Martin

[Mike McGrath]

On Thu, 25 Jun 2009, Todd Zullinger wrote:

>  Mike McGrath wrote:
>  > I'll take a look at this tomorrow, we've got a git check in there
>  > now that does a syntax and notify.  I think the only reason it
>  > prevents commits is because I didn't know how to do that :)  so all
>  > it does is throw errors.
>
>  That's in syncPuppetMaster.sh, called from the post-update hook,
>  right?  By then, there is no chance to deny the push, as the refs have
>  been updated by git. :)
>
>  > Here's the only got'cha.  We mix a private and public repo together.
>  > IE: in our public repo we reference $someDbPassword, and then in the
>  > private repo we create that password.  The only time they're
>  > together is after a push has happened.  Does this account for that?
>  > Does that problem not even exist anymore?
>
>  Using the code for the update hook in my previous mail, I don't think
>  is should be a problem.  That should only check the files that are
>  being modified by the push for syntax errors.  Puppet is called with
>  --parseonly and --ignoreimport.  That should prevent problems caused
>  by a manifest in puppet relying on something in private.  Of course,
>  testing it on a manifest that uses a variable define in private would
>  be a good idea. :)
>
>  Keeping the syntax check in syncPuppetMaster.sh is probably a good
>  backup, as it might catch things that the check on individual .pp
>  files misses.
>  
Works for me, patch seems resonable (if it does work like it seems it
should :)

Ping me on irc and we'll get this in and ready and tested.

	-Mike

[Todd Zullinger]

Mike McGrath wrote:
>  Works for me, patch seems resonable (if it does work like it seems
>  it should :)
>
>  Ping me on irc and we'll get this in and ready and tested. 
This is done now, we've moved the mail notification bits out of the
update hook and into the post-receive hook.  I'm sure many of you
noticed the changes in the mail formatting and perhaps found your
filters needed changed.

After making the initial change to use the upstream post-receive-email
hook yesterday, Owen Taylor pointed out that the gnome.org admins have
written a very nice post-receive-email script for use on the gnome.org
repositories¹.  I've added that script to puppet and installed it on
puppet1.  So the format of the commit mails should be a bit less
verbose once again.

In place of the X-Git-Module header, a Keywords header exists and
contains the git module name.  I also added the X-Project header with
the project description, in case anyone was using that to filter.

We're now also running the syntax check in the update hook, which
should prevent pushing changes with obvious syntax errors.  The output
of such a failure looks like this:

    http://fpaste.org/paste/16624

I still find it convenient to have a pre-commit hook that checks
syntax.  That keeps me from committing obviously broken things to my
local repo.  The hooks I whipped up and have been using on my local
puppet boxes for a while now are at:

    http://tmz.fedorapeople.org/tmp/pre-commit
    http://tmz.fedorapeople.org/tmp/pre-commit-puppet

If anyone notices problems with this, don't hesitate to yell at me via
mail or irc (tmz).

¹ http://git.gnome.org/cgit/gitadmin-bin/tree/gnome-post-receive-email
  (I did make a few small modifications to this script, all of which I
  sent to Owen for potential inclusion in the gitadmin-bin repo.  The
  most useful of which is encoding of user names in utf-8, where
  needed.  I tested this on fedorapeople.org and out of the 2124
  accounts there, 97 names were non-ascii and required encoding.)

-- 
Todd        OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Start every day with a smile and get it over with.
    -- W.C. Fields