Repository: audreyr/cookiecutter
Pull request #549 (2015-10-08 01:36:07)

Stop project generation if pre hook script fails
Hey folks,

Would this be an adequate solution to address issue #464 ?

My use case is handling an invalid package name (see https://github.com/audreyr/cookiecutter-pypackage/issues/6)

This proposal will stop the project generation if the pre hook script returns with an exit status different from 0 (indicating success).

With this proposal, we'd be able to handle that case with a `pre_gen_project` hook script like:

```
import re, sys

PACKAGE_REGEX = r'^[_a-zA-Z][_a-zA-Z0-9]+$'

repo_name = '{{ cookiecutter.repo_name }}'
if not re.match(PACKAGE_REGEX, repo_name):
    print('ERROR: %s is not a valid package name!' % repo_name)
    sys.exit(1)
```

Thoughts?

codecov-io: ## [Current coverage][1] is `99.17%`
> Merging **#549** into **master** will decrease coverage by **-0.41%** as of [`e5289dc`][3]


```diff
@@            master    #549   diff @@
======================================
  Files           12      12       
  Stmts          482     485     +3
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            480     481     +1
  Partial          0       0       
- Missed           2       4     +2
```

> Review entire [Coverage Diff][4] as of [`e5289dc`][3]


-----
### [Uncovered Suggestions][2]

1. `+0.41%` via [...iecutter/generate.py#261...262](https://codecov.io/github/audreyr/cookiecutter/cookiecutter/generate.py?ref=e5289dccffd883b13f986c84cb88936bc0322d21#261) 
1. `+0.41%` via [...ookiecutter/utils.py#27...28](https://codecov.io/github/audreyr/cookiecutter/cookiecutter/utils.py?ref=e5289dccffd883b13f986c84cb88936bc0322d21#27) 


[1]: https://codecov.io/github/audreyr/cookiecutter?ref=e5289dccffd883b13f986c84cb88936bc0322d21
[2]: https://codecov.io/github/audreyr/cookiecutter/features/suggestions?ref=e5289dccffd883b13f986c84cb88936bc0322d21
[3]: https://codecov.io/github/audreyr/cookiecutter/commit/e5289dccffd883b13f986c84cb88936bc0322d21
[4]: https://codecov.io/github/audreyr/cookiecutter/compare/066aa6499e367a73b28d24e2388febee3eb5d601...e5289dccffd883b13f986c84cb88936bc0322d21

> Powered by [Codecov](https://codecov.io). Updated on successful CI builds.
eliasdorneles: Okay, coverage bot, I'll add tests and improve coverage once I get some consensus that this is a good-enough approach. :)
hackebrot: Hi @eliasdorneles, thank you for your contribution! :bow: 

Tbh, I'd rather raise an error instead of returning ``0``. If I am not mistaken there is no error report outside of ``generate`` that the generation failed apart from a log entry. 
eliasdorneles: Hi @hackebrot :)
Right, my first try was raising a custom `HookFailedException`, but then I wasn't sure where it should be caught.
Also, it would raise for all hooks (not only for `pre_gen_project`), but maybe that'd be the desired behavior?
eliasdorneles: Hey, sorry for the delay -- I updated the code to use a custom exception and added cleaning up the project dir when aborting the generation, and added a little test.
Does this look better?
eliasdorneles: Note: tests are broken because of issue addressed in this PR: https://github.com/audreyr/cookiecutter/pull/555
eliasdorneles: Rebased to incorporate tests fix -- would appreciate some feedback. :)

I'm particularly not sure about what should be the handling the `FailedHookException` for the `post_gen_project` case -- which is why I didn't code anything for it.
hackebrot: I'd say we handle errors for ``post_gen_project`` in the same fashion. :no_mouth: 

More importantly, I feel we should definitely re-raise the exception after cleaning up the filesystem instead of [returning None](https://github.com/audreyr/cookiecutter/pull/549/files#diff-b4c1da8a76ea242bf99a397dcb90b2a6R267). We can catch the ``FailedHookException`` in the callers and ``sys.exit(1)``.

cc @michaeljoseph @audreyr @pydanny 
eliasdorneles: Thanks for reviewing @hackebrot -- I updated it with your suggestions. :)

Only thing, I think it's best not to clean up the filesystem for `post_gen_project` in the same fashion as for `pre_gen_project`. As it is a failure _after_ the project has been generated, the user wouldn't expect it to go away, and removing the project might make harder for users debug the hook scripts for their templates.
hackebrot: IMHO if a ``post_gen_project`` hook fails, the generated project is likely to corrupt regardless of the actual purpose of the hook. Even if it is just debug logging messages, I expect the hook to exit without an error.

For instance, if the [rename_kv_file hook](https://github.com/hackebrot/cookiedozer/blob/master/hooks/post_gen_project.py) fails, the generated kivy app is corrupt and cannot be launched. :worried: 

So the question is why would you want to keep a broken project? I feel it is rather up to hook to use comprehensive logging messages to help users debug any error that might occur.
eliasdorneles: Well, I don't have much experience using `post_gen_project` hooks, so if you say that when it fails it's likely the project to be broken, I trust you. :)

I think I was worrying unnecessarily, so lemme update this.

Patch:
diff --git a/cookiecutter/cli.py b/cookiecutter/cli.py
index 6f53e24..396ec0a 100755
--- a/cookiecutter/cli.py
+++ b/cookiecutter/cli.py
@@ -17,7 +17,7 @@ import click
 from cookiecutter import __version__
 from cookiecutter.main import cookiecutter
 from cookiecutter.exceptions import (
-    OutputDirExistsException, InvalidModeException
+    OutputDirExistsException, InvalidModeException, FailedHookException
 )
 
 logger = logging.getLogger(__name__)
@@ -81,9 +81,11 @@ def main(template, no_input, checkout, verbose, replay, overwrite_if_exists,
             overwrite_if_exists=overwrite_if_exists,
             output_dir=output_dir
         )
-    except (OutputDirExistsException, InvalidModeException) as e:
+    except (OutputDirExistsException,
+            InvalidModeException, FailedHookException) as e:
         click.echo(e)
         sys.exit(1)
 
+
 if __name__ == "__main__":
     main()
diff --git a/cookiecutter/exceptions.py b/cookiecutter/exceptions.py
index 0ad6b58..415f99e 100755
--- a/cookiecutter/exceptions.py
+++ b/cookiecutter/exceptions.py
@@ -81,3 +81,9 @@ class InvalidModeException(CookiecutterException):
     Raised when cookiecutter is called with both `no_input==True` and
     `replay==True` at the same time.
     """
+
+
+class FailedHookException(CookiecutterException):
+    """
+    Raised when a hook script fails
+    """
diff --git a/cookiecutter/generate.py b/cookiecutter/generate.py
index 94e6e94..7a4d461 100755
--- a/cookiecutter/generate.py
+++ b/cookiecutter/generate.py
@@ -24,10 +24,11 @@ from binaryornot.check import is_binary
 from .exceptions import (
     NonTemplatedInputDirException,
     ContextDecodingException,
+    FailedHookException,
     OutputDirExistsException
 )
 from .find import find_template
-from .utils import make_sure_path_exists, work_in
+from .utils import make_sure_path_exists, work_in, rmtree
 from .hooks import run_hook
 
 
@@ -222,6 +223,20 @@ def ensure_dir_is_templated(dirname):
         raise NonTemplatedInputDirException
 
 
+def _run_hook_from_repo_dir(repo_dir, hook_name, project_dir, context):
+    """
+    Run hook from repo directory, cleaning up project directory if hook fails
+    """
+    with work_in(repo_dir):
+        try:
+            run_hook(hook_name, project_dir, context)
+        except FailedHookException:
+            rmtree(project_dir)
+            logging.error("Stopping generation because %s"
+                          " hook script didn't exit sucessfully" % hook_name)
+            raise
+
+
 def generate_files(repo_dir, context=None, output_dir='.',
                    overwrite_if_exists=False):
     """
@@ -255,9 +270,7 @@ def generate_files(repo_dir, context=None, output_dir='.',
     project_dir = os.path.abspath(project_dir)
     logging.debug('project_dir is {0}'.format(project_dir))
 
-    # run pre-gen hook from repo_dir
-    with work_in(repo_dir):
-        run_hook('pre_gen_project', project_dir, context)
+    _run_hook_from_repo_dir(repo_dir, 'pre_gen_project', project_dir, context)
 
     with work_in(template_dir):
         env = Environment(keep_trailing_newline=True)
@@ -313,8 +326,6 @@ def generate_files(repo_dir, context=None, output_dir='.',
                 logging.debug('f is {0}'.format(f))
                 generate_file(project_dir, infile, context, env)
 
-    # run post-gen hook from repo_dir
-    with work_in(repo_dir):
-        run_hook('post_gen_project', project_dir, context)
+    _run_hook_from_repo_dir(repo_dir, 'post_gen_project', project_dir, context)
 
     return project_dir
diff --git a/cookiecutter/hooks.py b/cookiecutter/hooks.py
index 37d1764..81045d1 100755
--- a/cookiecutter/hooks.py
+++ b/cookiecutter/hooks.py
@@ -18,12 +18,15 @@ import tempfile
 from jinja2 import Template
 
 from cookiecutter import utils
+from .exceptions import FailedHookException
+
 
 _HOOKS = [
     'pre_gen_project',
     'post_gen_project',
     # TODO: other hooks should be listed here
 ]
+EXIT_SUCCESS = 0
 
 
 def find_hooks():
@@ -67,7 +70,10 @@ def run_script(script_path, cwd='.'):
         shell=run_thru_shell,
         cwd=cwd
     )
-    proc.wait()
+    exit_status = proc.wait()
+    if exit_status != EXIT_SUCCESS:
+        raise FailedHookException(
+            "Hook script failed (exit status: %d)" % exit_status)
 
 
 def run_script_with_context(script_path, cwd, context):
@@ -104,4 +110,4 @@ def run_hook(hook_name, project_dir, context):
     if script is None:
         logging.debug('No hooks found')
         return
-    return run_script_with_context(script, project_dir, context)
+    run_script_with_context(script, project_dir, context)
diff --git a/docs/advanced_usage.rst b/docs/advanced_usage.rst
index 2809db7..cee1b6a 100644
--- a/docs/advanced_usage.rst
+++ b/docs/advanced_usage.rst
@@ -34,6 +34,35 @@ hooks, as these can be run on any platform. However, if you intend for your
 template to only be run on a single platform, a shell script (or `.bat` file
 on Windows) can be a quicker alternative.
 
+.. note::
+    Make sure your hook scripts work in a robust manner. If a hook script fails
+    (that is, `if it finishes with a nonzero exit status
+    <https://docs.python.org/3/library/sys.html#sys.exit>`_), the project
+    generation will stop and the generated directory will be cleaned up.
+
+Example: Validating template variables
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Here is an example of script that validates a template variable
+before generating the project, to be used as ``hooks/pre_gen_project.py``:
+
+.. code-block:: python
+
+    import re
+    import sys
+
+
+    MODULE_REGEX = r'^[_a-zA-Z][_a-zA-Z0-9]+$'
+
+    module_name = '{{ cookiecutter.module_name }}'
+
+    if not re.match(MODULE_REGEX, module_name):
+        print('ERROR: %s is not a valid Python module name!' % module_name)
+
+        # exits with status 1 to indicate failure
+        sys.exit(1)
+
+
 User Config (0.7.0+)
 ----------------------
 


Test patch:
diff --git a/tests/test_hooks.py b/tests/test_hooks.py
index 8df394f..8583dbe 100755
--- a/tests/test_hooks.py
+++ b/tests/test_hooks.py
@@ -11,8 +11,9 @@ Tests for `cookiecutter.hooks` module.
 import sys
 import os
 import stat
+import pytest
 
-from cookiecutter import hooks, utils
+from cookiecutter import hooks, utils, exceptions
 
 
 def make_test_repo(name):
@@ -166,3 +167,16 @@ class TestExternalHooks(object):
 
             hooks.run_hook('post_gen_project', tests_dir, {})
             assert os.path.isfile(os.path.join(tests_dir, 'shell_post.txt'))
+
+    def test_run_failing_hook(self):
+        hook_path = os.path.join(self.hooks_path, 'pre_gen_project.py')
+        tests_dir = os.path.join(self.repo_path, 'input{{hooks}}')
+
+        with open(hook_path, 'w') as f:
+            f.write("#!/usr/bin/env python\n")
+            f.write("import sys; sys.exit(1)\n")
+
+        with utils.work_in(self.repo_path):
+            with pytest.raises(exceptions.FailedHookException) as excinfo:
+                hooks.run_hook('pre_gen_project', tests_dir, {})
+            assert 'Hook script failed' in str(excinfo.value)