From 5f392f0ba5e640ee105b6586f6d365401a0a2923 Mon Sep 17 00:00:00 2001 From: Ben Picolo Date: Tue, 3 Jan 2017 16:47:59 -0500 Subject: [PATCH 1/6] Docker hook support for pre-commit --- pre_commit/languages/all.py | 2 ++ pre_commit/languages/docker.py | 62 ++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 pre_commit/languages/docker.py diff --git a/pre_commit/languages/all.py b/pre_commit/languages/all.py index 40c23131..a517ee85 100644 --- a/pre_commit/languages/all.py +++ b/pre_commit/languages/all.py @@ -1,5 +1,6 @@ from __future__ import unicode_literals +from pre_commit.languages import docker from pre_commit.languages import node from pre_commit.languages import pcre from pre_commit.languages import python @@ -40,6 +41,7 @@ from pre_commit.languages import system # """ languages = { + 'docker': docker, 'node': node, 'pcre': pcre, 'python': python, diff --git a/pre_commit/languages/docker.py b/pre_commit/languages/docker.py new file mode 100644 index 00000000..afb16831 --- /dev/null +++ b/pre_commit/languages/docker.py @@ -0,0 +1,62 @@ +from __future__ import unicode_literals + +import hashlib +import os + +from pre_commit.languages import helpers +from pre_commit.util import clean_path_on_failure +from pre_commit.util import mkdirp +from pre_commit.xargs import xargs + + +ENVIRONMENT_DIR = 'docker' + + +def md5(s): + m = hashlib.md5() + m.update(s) + return m.hexdigest() + + +def docker_tag(repo_cmd_runner): + return 'pre-commit-{}'.format( + md5(os.path.basename(repo_cmd_runner.path())) + ).lower() + + +def install_environment( + repo_cmd_runner, + version='default', + additional_dependencies=(), +): + assert repo_cmd_runner.exists('Dockerfile') + # I don't know of anybody trying to juggle multiple docker installations + # so this seems sufficient + directory = helpers.environment_dir(ENVIRONMENT_DIR, 'default') + mkdirp(os.path.join(repo_cmd_runner.path(), directory)) + + cmd = ( + 'docker', 'build', '--pull', + '--tag', docker_tag(repo_cmd_runner), + '.' + ) + + # Docker doesn't really have relevant disk environment, but pre-commit + # still needs to cleanup it's state files on failure + env_dir = repo_cmd_runner.path(directory) + with clean_path_on_failure(env_dir): + helpers.run_setup_cmd(repo_cmd_runner, cmd) + + +def run_hook(repo_cmd_runner, hook, file_args): + cmd = ( + 'docker', 'run', + '-t', + '-v', '{}:/src'.format(os.getcwd()), + '--workdir', '/src', + docker_tag(repo_cmd_runner) + ) + + return xargs( + cmd + (hook['entry'],) + tuple(hook['args']), file_args, + ) From 9b92f96ed0dd461dca6a37821af880719afdd350 Mon Sep 17 00:00:00 2001 From: Ben Picolo Date: Wed, 4 Jan 2017 10:35:00 -0500 Subject: [PATCH 2/6] Code cleanup and tests --- .travis.yml | 4 +- CONTRIBUTING.md | 5 +- pre_commit/languages/docker.py | 60 +++++++++++++------ .../resources/docker_hooks_repo/Dockerfile | 3 + .../resources/docker_hooks_repo/hooks.yaml | 11 ++++ testing/util.py | 6 ++ tests/repository_test.py | 24 ++++++++ 7 files changed, 92 insertions(+), 21 deletions(-) create mode 100644 testing/resources/docker_hooks_repo/Dockerfile create mode 100644 testing/resources/docker_hooks_repo/hooks.yaml diff --git a/.travis.yml b/.travis.yml index c5f989b5..a01003c3 100644 --- a/.travis.yml +++ b/.travis.yml @@ -20,7 +20,9 @@ before_install: - git --version after_success: - coveralls -sudo: false +sudo: required +services: + - docker cache: directories: - $HOME/.cache/pip diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a5170902..1e0b2460 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -2,7 +2,7 @@ ## Local development -- The tests depend on having at least the following installed (possibly not +- The complete test suite depends on having at least the following installed (possibly not a complete list) - git (A sufficiently newer version is required to run pre-push tests) - python @@ -10,6 +10,7 @@ - python3.5 (Required by a test which checks different python versions) - tox (or virtualenv) - ruby + gem + - docker ### Setting up an environemnt @@ -63,7 +64,7 @@ function_call( ``` Some notable features: -- The intial parenthese is at the end of the line +- The initial parenthesis is at the end of the line - Parameters are indented one indentation level further than the function name - The last parameter contains a trailing comma (This helps make `git blame` more accurate and reduces merge conflicts when adding / removing parameters). diff --git a/pre_commit/languages/docker.py b/pre_commit/languages/docker.py index afb16831..bc4d0623 100644 --- a/pre_commit/languages/docker.py +++ b/pre_commit/languages/docker.py @@ -1,8 +1,10 @@ +from __future__ import absolute_import from __future__ import unicode_literals import hashlib import os +from pre_commit import five from pre_commit.languages import helpers from pre_commit.util import clean_path_on_failure from pre_commit.util import mkdirp @@ -10,12 +12,11 @@ from pre_commit.xargs import xargs ENVIRONMENT_DIR = 'docker' +PRE_COMMIT_LABEL = 'PRE_COMMIT' def md5(s): - m = hashlib.md5() - m.update(s) - return m.hexdigest() + return five.to_text(hashlib.md5(s).hexdigest()) def docker_tag(repo_cmd_runner): @@ -24,39 +25,62 @@ def docker_tag(repo_cmd_runner): ).lower() +def docker_is_running(): + return xargs(('docker',), ['ps'])[0] == 0 + + +def assert_docker_available(): + assert docker_is_running(), ( + 'Docker is either not running or not configured in this environment' + ) + + +def build_docker_image(repo_cmd_runner): + cmd = ( + 'docker', 'build', '--pull', + '--tag', docker_tag(repo_cmd_runner), + '--label', PRE_COMMIT_LABEL, + '.' + ) + helpers.run_setup_cmd(repo_cmd_runner, cmd) + + def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), ): - assert repo_cmd_runner.exists('Dockerfile') - # I don't know of anybody trying to juggle multiple docker installations - # so this seems sufficient + assert repo_cmd_runner.exists('Dockerfile'), ( + 'No Dockerfile was found in the hook repository' + ) + assert version == 'default', ( + 'Pre-commit does not support language_version for docker ' + ) + assert_docker_available() + directory = helpers.environment_dir(ENVIRONMENT_DIR, 'default') mkdirp(os.path.join(repo_cmd_runner.path(), directory)) - cmd = ( - 'docker', 'build', '--pull', - '--tag', docker_tag(repo_cmd_runner), - '.' - ) - # Docker doesn't really have relevant disk environment, but pre-commit # still needs to cleanup it's state files on failure env_dir = repo_cmd_runner.path(directory) with clean_path_on_failure(env_dir): - helpers.run_setup_cmd(repo_cmd_runner, cmd) + build_docker_image(repo_cmd_runner) def run_hook(repo_cmd_runner, hook, file_args): + assert_docker_available() + # Rebuild the docker image in case it has gone missing, as many people do + # automated cleanup of docker images. + build_docker_image(repo_cmd_runner) + # the docker lib doesn't return stdout on non-zero exit codes, + # so we run the container directly on the command line cmd = ( 'docker', 'run', - '-t', + '--rm', '-v', '{}:/src'.format(os.getcwd()), '--workdir', '/src', + '--entrypoint', hook['entry'], docker_tag(repo_cmd_runner) ) - - return xargs( - cmd + (hook['entry'],) + tuple(hook['args']), file_args, - ) + return xargs(cmd + tuple(hook['args']), file_args) diff --git a/testing/resources/docker_hooks_repo/Dockerfile b/testing/resources/docker_hooks_repo/Dockerfile new file mode 100644 index 00000000..acb5f54e --- /dev/null +++ b/testing/resources/docker_hooks_repo/Dockerfile @@ -0,0 +1,3 @@ +FROM cogniteev/echo + +CMD ["echo", "This is overwritten by the hooks.yaml 'entry'"] diff --git a/testing/resources/docker_hooks_repo/hooks.yaml b/testing/resources/docker_hooks_repo/hooks.yaml new file mode 100644 index 00000000..2c9a115d --- /dev/null +++ b/testing/resources/docker_hooks_repo/hooks.yaml @@ -0,0 +1,11 @@ +- id: docker-hook + name: Docker test hook + entry: echo + language: docker + files: \.txt$ + +- id: docker-hook-failing + name: Docker test hook with nonzero exit code + entry: bork + language: docker + files: \.txt$ diff --git a/testing/util.py b/testing/util.py index 10e84462..cf9dde9d 100644 --- a/testing/util.py +++ b/testing/util.py @@ -6,6 +6,7 @@ import shutil import jsonschema import pytest +from pre_commit.languages.docker import docker_is_running from pre_commit.util import cmd_output from pre_commit.util import cwd @@ -57,6 +58,11 @@ def cmd_output_mocked_pre_commit_home(*args, **kwargs): return cmd_output(*args, env=env, **kwargs) +skipif_cant_run_docker = pytest.mark.skipif( + docker_is_running() is False, + reason='Docker isn\'t running or can\'t be accessed' +) + skipif_slowtests_false = pytest.mark.skipif( os.environ.get('slowtests') == 'false', reason='slowtests=false', diff --git a/tests/repository_test.py b/tests/repository_test.py index f61ee88d..5d8ed8ab 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -29,6 +29,7 @@ from testing.fixtures import git_dir from testing.fixtures import make_config_from_repo from testing.fixtures import make_repo from testing.fixtures import modify_manifest +from testing.util import skipif_cant_run_docker from testing.util import skipif_slowtests_false from testing.util import xfailif_no_pcre_support from testing.util import xfailif_windows_no_node @@ -129,6 +130,29 @@ def test_versioned_python_hook(tempdir_factory, store): ) +@skipif_slowtests_false +@skipif_cant_run_docker +@pytest.mark.integration +def test_run_a_docker_hook(tempdir_factory, store): + _test_hook_repo( + tempdir_factory, store, 'docker_hooks_repo', + 'docker-hook', + ['Hello World from docker'], b'Hello World from docker\n', + ) + + +@skipif_slowtests_false +@skipif_cant_run_docker +@pytest.mark.integration +def test_run_a_failing_docker_hook(tempdir_factory, store): + _test_hook_repo( + tempdir_factory, store, 'docker_hooks_repo', + 'docker-hook-failing', + ['Hello World from docker'], b'', + expected_return_code=1 + ) + + @skipif_slowtests_false @xfailif_windows_no_node @pytest.mark.integration From baf254ab788d2ca02331c418c6ff2eb0aae687ed Mon Sep 17 00:00:00 2001 From: Ben Picolo Date: Wed, 4 Jan 2017 10:44:18 -0500 Subject: [PATCH 3/6] Fix user so we can mount volumes as RW --- pre_commit/languages/docker.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pre_commit/languages/docker.py b/pre_commit/languages/docker.py index bc4d0623..e4e5af3e 100644 --- a/pre_commit/languages/docker.py +++ b/pre_commit/languages/docker.py @@ -78,9 +78,11 @@ def run_hook(repo_cmd_runner, hook, file_args): cmd = ( 'docker', 'run', '--rm', - '-v', '{}:/src'.format(os.getcwd()), + '-u', '{}:{}'.format(os.getuid(), os.getgid()), + '-v', '{}:/src:rw'.format(os.getcwd()), '--workdir', '/src', '--entrypoint', hook['entry'], docker_tag(repo_cmd_runner) ) + return xargs(cmd + tuple(hook['args']), file_args) From 86c0e6d297a20f959389db9050d175d9e530d49b Mon Sep 17 00:00:00 2001 From: Ben Picolo Date: Wed, 4 Jan 2017 10:52:56 -0500 Subject: [PATCH 4/6] Inverse md5 bytesifying --- pre_commit/languages/docker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pre_commit/languages/docker.py b/pre_commit/languages/docker.py index e4e5af3e..e6f2d4e4 100644 --- a/pre_commit/languages/docker.py +++ b/pre_commit/languages/docker.py @@ -16,7 +16,7 @@ PRE_COMMIT_LABEL = 'PRE_COMMIT' def md5(s): - return five.to_text(hashlib.md5(s).hexdigest()) + return hashlib.md5(five.to_bytes(s)).hexdigest() def docker_tag(repo_cmd_runner): From b06da3e9cd9942118d8b982e4456a517967f8f4c Mon Sep 17 00:00:00 2001 From: Ben Picolo Date: Wed, 4 Jan 2017 11:57:27 -0500 Subject: [PATCH 5/6] Code review tweaks --- pre_commit/languages/docker.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/pre_commit/languages/docker.py b/pre_commit/languages/docker.py index e6f2d4e4..b2fed46d 100644 --- a/pre_commit/languages/docker.py +++ b/pre_commit/languages/docker.py @@ -6,8 +6,9 @@ import os from pre_commit import five from pre_commit.languages import helpers +from pre_commit.util import CalledProcessError from pre_commit.util import clean_path_on_failure -from pre_commit.util import mkdirp +from pre_commit.util import cmd_output from pre_commit.xargs import xargs @@ -26,7 +27,10 @@ def docker_tag(repo_cmd_runner): def docker_is_running(): - return xargs(('docker',), ['ps'])[0] == 0 + try: + return cmd_output('docker', 'ps')[0] == 0 + except CalledProcessError: + return False def assert_docker_available(): @@ -59,7 +63,7 @@ def install_environment( assert_docker_available() directory = helpers.environment_dir(ENVIRONMENT_DIR, 'default') - mkdirp(os.path.join(repo_cmd_runner.path(), directory)) + os.mkdir(repo_cmd_runner.path(directory)) # Docker doesn't really have relevant disk environment, but pre-commit # still needs to cleanup it's state files on failure @@ -73,8 +77,6 @@ def run_hook(repo_cmd_runner, hook, file_args): # Rebuild the docker image in case it has gone missing, as many people do # automated cleanup of docker images. build_docker_image(repo_cmd_runner) - # the docker lib doesn't return stdout on non-zero exit codes, - # so we run the container directly on the command line cmd = ( 'docker', 'run', '--rm', From 08b379bf458ee12b0f462ec41c71a82fa1ba8f9c Mon Sep 17 00:00:00 2001 From: Ben Picolo Date: Wed, 4 Jan 2017 13:16:32 -0500 Subject: [PATCH 6/6] Coverage complete --- tests/languages/docker_test.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 tests/languages/docker_test.py diff --git a/tests/languages/docker_test.py b/tests/languages/docker_test.py new file mode 100644 index 00000000..6ca2ed5c --- /dev/null +++ b/tests/languages/docker_test.py @@ -0,0 +1,15 @@ +from __future__ import absolute_import +from __future__ import unicode_literals + +import mock + +from pre_commit.languages import docker +from pre_commit.util import CalledProcessError + + +def test_docker_is_running_process_error(): + with mock.patch( + 'pre_commit.languages.docker.cmd_output', + side_effect=CalledProcessError(*(None,) * 4) + ): + assert docker.docker_is_running() is False