Mark Plaksin wrote: >> One potential fix for that is to check for deletions, like so: > > Thanks for doing work for us :) We noticed the need for this but > haven't had a chance to fix it. Your change works great. I updated the > Wiki.
Cool. Credit for that one goes to Ricky Zhou. When we added this the
Fedora Infrastructure puppet repo, he was the first one lucky enough
to get bitten by it. :)
>> One other potential problem is if puppet is used to manage selinux
>> modules. Compiled modules also have a .pp extension. When adding or
>> updating these files they will pass the "if [[ $name =~ [.]pp$ ]]"
>> check. This can be avoided by not version controlling the compiled
>> modules, but perhaps it might also be reasonable to add a quick bit to
>> the if test, something like this (untested):
>>
>> if [[ $name =~ [.]pp$ ]] && [ "$(file -b "$name" 2>/dev/null)" != 'data'
>> ]
>>
>> The selinux .pp files will return data, while I can't imaging any
>> puppet manifests being labeled as data. :)
>
> This sounds good to me but maybe it's not safe to assume GNU file (which
> supports the -b) is installed on your puppetmaster?
It might not be a safe assumption. Having any file command might not
be safe to assume, really. Worse, the test above wouldn't work
because $name would not be available as a file so the command would
fail. Here's something a little more likely to work (I did test this
one):
diff --git a/puppet-update-hook b/puppet-update-hook
index 539f969..74acaa7 100644
--- a/puppet-update-hook
+++ b/puppet-update-hook
@@ -40,6 +40,9 @@ do
if [[ $name =~ [.]pp$ ]]
then
git cat-file blob $new_sha1 > $tmp
+ # SELinux modules also have a .pp extension, try to skip them
+ ftype="$((file "$tmp" | awk -F': ' '{print $2}') 2>/dev/null)"
+ [ "$ftype" = "data" ] && continue
set -o pipefail
$syntax_check $tmp 2>&1 | sed
's|/tmp/git.update.......:\([0-9]*\)$|JOJOMOJO:\1|'> $log
if [[ $? != 0 ]]
If file isn't available, the hook might still try to syntax check a
non-puppet .pp file, but that's no worse than things are currently.
In testing, there are a few other things I noticed. When making an
initial push with this hook enabled, $2 is 000000, which causes git
diff-tree to yield an error. Fixing this would require a bit of
fiddling. I'm also wondering if there's any good reason to keep the
'echo diff-tree:; cat $tree' code? It looks like leftover debugging
to me.
One other potential improvement I have is to use shorter commit id's
when telling a user how to check the diff when a problem is found:
diff --git a/puppet-update-hook b/puppet-update-hook
index 74acaa7..38551ef 100644
--- a/puppet-update-hook
+++ b/puppet-update-hook
@@ -49,7 +49,7 @@ do
then
echo
echo -e "$(cat $log | sed 's|JOJOMOJO|'\\${RED}${name}\\${NOBOLD}'|')"
>&2
- echo -e "For more details run this: ${CYAN} git diff $old_sha1 $new_sha1
${NOBOLD}" >&2
+ echo -e "For more details run this: ${CYAN} git diff ${old_sha1:0:7}
${new_sha1:0:7} ${NOBOLD}" >&2
echo
exit_status=1
fi
--
Todd OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
It is impossible to enjoy idling thoroughly unless one has plenty of
work to do.
-- Jerome K. Jerome
pgpa8s86EZckh.pgp
Description: PGP signature
