Stop project generation if pre hook script fails #549

eliasdorneles · 2015-10-08T01:36:07Z

Hey folks,

Would this be an adequate solution to address issue #464 ?

My use case is handling an invalid package name (see audreyfeldroy/cookiecutter-pypackage#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 · 2015-10-08T01:41:56Z

Current coverage is `98.39%`

Merging #549 into master will decrease coverage by -0.78% as of 7141f6d

@@            master    #549   diff @@
======================================
  Files           12      12       
  Stmts          486     497    +11
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            482     489     +7
  Partial          0       0       
- Missed           4       8     +4

Review entire Coverage Diff as of 7141f6d

Uncovered Suggestions

+0.80% via ...iecutter/generate.py#233...236
+0.40% via ...okiecutter/config.py#19...20
+0.40% via ...ookiecutter/utils.py#27...28

Powered by Codecov. Updated on successful CI builds.

eliasdorneles · 2015-10-08T01:55:28Z

Okay, coverage bot, I'll add tests and improve coverage once I get some consensus that this is a good-enough approach. :)

hackebrot · 2015-10-08T19:48:03Z

Hi @eliasdorneles, thank you for your contribution! 🙇

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 · 2015-10-08T20:35:41Z

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 · 2015-10-17T05:49:46Z

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 · 2015-10-17T06:26:24Z

Note: tests are broken because of issue addressed in this PR: #555

eliasdorneles · 2015-10-18T04:10:40Z

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 · 2015-10-18T12:27:09Z

I'd say we handle errors for post_gen_project in the same fashion. 😶

More importantly, I feel we should definitely re-raise the exception after cleaning up the filesystem instead of returning None. We can catch the FailedHookException in the callers and sys.exit(1).

cc @michaeljoseph @audreyr @pydanny

hackebrot · 2015-10-18T12:27:54Z

tests/test_hooks.py

+        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)


I think assert 'Hook script failed' in excinfo.value should work too.

eliasdorneles · 2015-10-18T15:47:51Z

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 · 2015-10-20T18:32:14Z

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 fails, the generated kivy app is corrupt and cannot be launched. 😟

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 · 2015-10-20T18:39:38Z

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.

eliasdorneles · 2015-10-20T19:00:14Z

Does this look better?

hackebrot · 2015-10-20T21:00:31Z

cookiecutter/generate.py

+    """
+    with work_in(repo_dir):
+        try:
+            run_hook('pre_gen_project', project_dir, context)


I guess 'pre_gen_project' is supposed to be hook_name. 😶

ooops, lemme fix that! :D

hackebrot · 2015-10-20T22:09:52Z

tests/test_hooks.py

+        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:


Uhhm, are you changing the test files at runtime? This may break other tests unintentionally... 😟

self.repo_path is destroyed in teardown_method and recreated in setup_method for each test:

https://github.com/audreyr/cookiecutter/pull/549/files#diff-392f380cd08f2c7cc3ac39b10cf80d95R94

You are right 😪

hackebrot · 2015-10-20T22:34:44Z

Great work @eliasdorneles 👍

Let's see what the others think 😸

eliasdorneles · 2015-10-20T22:35:39Z

Cool, thanks for reviewing @hackebrot ! :)

pydanny · 2015-10-21T19:33:01Z

cookiecutter/hooks.py

@@ -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)


Why remove the ability to return a value?

well, it was meant as a cleanup, run_script_with_context was never returning anything, so I removed the return to make clear this function is a command not a query.

But I can revert the change, if you have other plans or I missed something. :)

pydanny · 2015-10-21T19:34:40Z

This is a really nice feature that deserves documentation ;)

eliasdorneles · 2015-10-21T20:28:29Z

Agreed, lemme add some docs! :)

eliasdorneles · 2015-10-21T21:01:57Z

I added a note under the hook section -- does it look good?

hackebrot · 2015-10-21T21:04:43Z

docs/advanced_usage.rst

+.. note::
+    Make sure your hook scripts work in a robust manner.  If a hook script
+    fails (that is, if it finishes with an exit status that is not one of
+    success), the project generation will stop and the generated directory will


...with an exit status that is not 0... ❔

right, I was unsure if mentioning 0 would be more clear or more confusing.
I guess 0 is more clear. :)

You could refer to the official sys.exit docs to be very explicit.

If it is an integer, zero is considered “successful termination” and any nonzero value is considered “abnormal termination” by shells and the like.

Right, though the comment applies to shell script hooks too.
I'll link to it.

hackebrot · 2015-10-21T21:06:24Z

I think the example that you provided in the description of the PR would be a great example for using this new feature. Let's add it to the docs 😉

(if @eliasdorneles and @pydanny agree 😁 )

eliasdorneles · 2015-10-21T22:21:59Z

I've added an example and improved the wording -- looks good now?

hackebrot · 2015-10-21T22:40:06Z

docs/advanced_usage.rst

+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Here is an example of script that validates a template variable
+before generating the project, to be used as ``hooks/pre_gen_project.py``::


I feel sorry for being this picky... 🙊

PEP8 Imports recommends the following:

import re import sys

Also you could enable syntax highlighting in rst via:

to be used as ``hooks/pre_gen_project.py``: .. code-block:: python import re

haha, I don't mind the nitpicking, keep them coming. :)

who doesn't enjoy some bikeshedding? 🚲 🎪 :D

although... python highlighting works for me without that markup. 😕

done!

PS: wow, vim highlights sample code inline with .. code-block:: python 😮 :notbad:

luzfcb · 2015-10-22T17:13:30Z

docs/advanced_usage.rst

+.. 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/2/library/sys.html#sys.exit>`_), the project


@eliasdorneles It would be nice to indicate the Python3 documentation instead of Legacy Python.

hackebrot · 2015-10-26T23:44:36Z

😸 👍

eliasdorneles · 2015-10-29T16:00:11Z

Anything else, fellows?
Do you prefer I squash the commits?

hackebrot · 2015-10-29T17:16:45Z

No, you're fine! 👍

We are just super busy atm. Apologies for the delay. 😟

Going to merge this guy once we get another upvote from a contributor.

eliasdorneles · 2015-10-29T17:37:16Z

gotcha!

btw, in Scrapy we find useful to rename the PR prefixing with [MRG+1] for these cases (PRs that were already reviewed and are only waiting for another upvote). =)

pydanny · 2015-11-05T15:51:49Z

LGTM! 👍

Stop project generation if pre hook script fails

hackebrot · 2015-11-05T16:42:40Z

Great work! 👍 👍

eliasdorneles · 2015-11-05T17:14:24Z

Thank you! =)

hackebrot · 2015-11-07T01:06:49Z

I just realized we missed sth 😢

We need coverage for cookiecutter.generate:

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

hackebrot · 2015-11-07T01:30:38Z

Do you mind submitting a PR @eliasdorneles?

eliasdorneles · 2015-11-07T01:44:08Z

oh, right.
sure, but it'll take me a few days, I'm on the road to Python Brasil. :)
I'll look at it after the conference.
On Nov 6, 2015 11:30 PM, "Raphael Pierzina" notifications@github.com
wrote:

Do you mind submitting a PR @eliasdorneles
https://github.com/eliasdorneles?

—
Reply to this email directly or view it on GitHub
#549 (comment).

hackebrot · 2015-11-08T01:24:13Z

No worries! Have fun 😸

#580

eliasdorneles force-pushed the stop-build-if-pre-hook-fails branch 2 times, most recently from b17f77f to e19b739 Compare October 18, 2015 04:07

hackebrot reviewed Oct 18, 2015
View reviewed changes

hackebrot added enhancement This issue/PR relates to a feature request. needs-review PR Only: This PR require review from other developer labels Oct 18, 2015

eliasdorneles added 5 commits October 20, 2015 16:44

stop project generation if pre hook script fails

9568ab6

using custom exception instead of exit status and cleanup project dir

457a1a4

assert on exception instance

290962b

handle failed hook exception in cli

5eafab8

handle post_gen_project hook the same way as pre_gen_project

74ed88e

eliasdorneles force-pushed the stop-build-if-pre-hook-fails branch from 2340b86 to 74ed88e Compare October 20, 2015 18:59

hackebrot reviewed Oct 20, 2015
View reviewed changes

fix hardcoded value

855267b

hackebrot reviewed Oct 20, 2015
View reviewed changes

pydanny reviewed Oct 21, 2015
View reviewed changes

add note about canceling project generation when hook script fails

8fb94a6

hackebrot reviewed Oct 21, 2015
View reviewed changes

eliasdorneles added 2 commits October 21, 2015 20:15

added hook script example

48ef69b

clarify bit about exit status

724869b

hackebrot reviewed Oct 21, 2015
View reviewed changes

apply python syntax highlight explicitly, PEP-8 example code

7f7284e

luzfcb reviewed Oct 22, 2015
View reviewed changes

pointing to python 3 library docs

f823f39

hackebrot added the needs-docs PR Only: This PR require additional documentation label Oct 22, 2015

hackebrot added a commit that referenced this pull request Nov 5, 2015

Merge pull request #549 from eliasdorneles/stop-build-if-pre-hook-fails

5b997e8

Stop project generation if pre hook script fails

hackebrot merged commit 5b997e8 into cookiecutter:master Nov 5, 2015

hackebrot added a commit that referenced this pull request Nov 5, 2015

Update HISTORY.rst with changes of #549

1b44efc

hackebrot mentioned this pull request Nov 5, 2015

Stop build if pre hook fails #464

Closed

hackebrot mentioned this pull request Nov 7, 2015

Test Coverage for _run_hook_from_repo_dir #579

Closed

hackebrot mentioned this pull request Feb 11, 2016

Exposing the configured context to the hooks. #321

Closed

Stop project generation if pre hook script fails #549

Stop project generation if pre hook script fails #549

Conversation

eliasdorneles commented Oct 8, 2015

codecov-io commented Oct 8, 2015

Current coverage is 98.39%

eliasdorneles commented Oct 8, 2015

hackebrot commented Oct 8, 2015

eliasdorneles commented Oct 8, 2015

eliasdorneles commented Oct 17, 2015

eliasdorneles commented Oct 17, 2015

eliasdorneles commented Oct 18, 2015

hackebrot commented Oct 18, 2015

Choose a reason for hiding this comment

eliasdorneles commented Oct 18, 2015

hackebrot commented Oct 20, 2015

eliasdorneles commented Oct 20, 2015

eliasdorneles commented Oct 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackebrot commented Oct 20, 2015

eliasdorneles commented Oct 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pydanny commented Oct 21, 2015

eliasdorneles commented Oct 21, 2015

eliasdorneles commented Oct 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackebrot commented Oct 21, 2015

eliasdorneles commented Oct 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackebrot commented Oct 26, 2015

eliasdorneles commented Oct 29, 2015

hackebrot commented Oct 29, 2015

eliasdorneles commented Oct 29, 2015

pydanny commented Nov 5, 2015

hackebrot commented Nov 5, 2015

eliasdorneles commented Nov 5, 2015

hackebrot commented Nov 7, 2015

hackebrot commented Nov 7, 2015

eliasdorneles commented Nov 7, 2015

hackebrot commented Nov 8, 2015

Current coverage is `98.39%`