Changed logic: hooks no longer get the payload as argument, now they get the path to a file holding the payload.

carlos-jenkins · carlos-jenkins · commit 64bd54bea619 · 2014-09-23T18:08:04.000-06:00
Passing the payload as an argument was too error prone.
diff --git a/README.rst b/README.rst
@@ -58,9 +58,16 @@ following order:
     hooks/{event}
     hooks/all
 
-The application will pass to the hooks the JSON received as first argument.
-Hooks can be written in any scripting language as long as the file is executable
-and has a shebang. A simple example in Python could be:
+The application will pass to the hooks the path to a JSON file holding the
+payload for the request as first argument. The event type will be passed
+as second argument. For example:
+
+::
+
+    hooks/push-myrepo-master /tmp/sXFHji push
+
+Hooks can be written in any scripting language as long as the file is
+executable and has a shebang. A simple example in Python could be:
 
 ::
 
@@ -71,7 +78,8 @@ and has a shebang. A simple example in Python could be:
     import sys
     import json
 
-    payload = json.loads(sys.argv[1])
+    with open(sys.argv[1], 'r') as jsf:
+      payload = json.loads(jsf.read())
 
     ### Do something with the payload
     name = payload['repository']['name']
diff --git a/webhooks.py b/webhooks.py
@@ -23,7 +23,8 @@
 from hashlib import sha1
 from json import loads, dumps
 from subprocess import Popen, PIPE
-from os import access, X_OK
+from tempfile import mkstemp
+from os import access, X_OK, remove
 from os.path import isfile, abspath, normpath, dirname, join, basename
 
 import requests
@@ -99,31 +100,48 @@ def index():
         join(hooks, 'all')
     ]
 
+    # Check permissions
+    scripts = [s for s in scripts if isfile(s) and access(s, X_OK)]
+    if not scripts:
+        return ''
+
+    # Save payload to temporal file
+    _, tmpfile = mkstemp()
+    with open(tmpfile, 'w') as pf:
+        pf.write(dumps(payload))
+
     # Run scripts
     ran = {}
     for s in scripts:
-        if isfile(s) and access(s, X_OK):
 
-            proc = Popen(
-                [s, dumps(payload)],
-                shell=True,
-                stdout=PIPE, stderr=PIPE
-            )
-            stdout, stderr = proc.communicate()
+        proc = Popen(
+            [s, tmpfile, event],
+            shell=True,
+            stdout=PIPE, stderr=PIPE
+        )
+        stdout, stderr = proc.communicate()
+
+        ran[basename(s)] = {
+            'returncode': proc.returncode,
+            'stdout': stdout,
+            'stderr': stderr,
+        }
 
-            ran[basename(s)] = {
-                'returncode': proc.returncode,
-                'stdout': stdout,
-                'stderr': stderr,
-            }
+        # Log errors if a hook failed
+        if proc.returncode != 0:
+            logging.error('{} : {} \n{}'.format(
+                s, proc.returncode, stderr
+            ))
 
-    output = ''
+    # Remove temporal file
+    remove(tmpfile)
 
     info = config.get('return_scripts_info', False)
-    if info:
-        output = dumps(ran, sort_keys=True, indent=4)
-        logging.debug(output)
+    if not info:
+        return ''
 
+    output = dumps(ran, sort_keys=True, indent=4)
+    logging.info(output)
     return output
 
 
