From 4e1eff5f1bab103e65a631050a3c4ef84429c5ec Mon Sep 17 00:00:00 2001 From: Tim Head Date: Tue, 16 Oct 2018 08:22:19 +0200 Subject: [PATCH 1/3] Switch repository used to test sub-directory support Using a repository that contains invlid instructions in the root of the repository so that the test for subdirectory support will fail if repo2docker doesn't actually switch into the requested sub-directory. --- repo2docker/app.py | 7 ++++--- tests/test_subdir.py | 19 +++++++++++-------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/repo2docker/app.py b/repo2docker/app.py index e8e65ce8..04f0b560 100644 --- a/repo2docker/app.py +++ b/repo2docker/app.py @@ -704,9 +704,10 @@ class Repo2Docker(Application): self.fetch(self.repo, self.ref, checkout_path) if self.subdir: - checkout_path = os.path.join(checkout_path, self.subdir).rstrip('/') - if not os.path.exists(checkout_path): - self.log.error('Subdirectory %s does not exist', self.subdir, extra=dict(phase='failure')) + checkout_path = os.path.join(checkout_path, self.subdir) + if not os.path.isdir(checkout_path): + self.log.error('Subdirectory %s does not exist', + self.subdir, extra=dict(phase='failure')) sys.exit(1) os.chdir(checkout_path) diff --git a/tests/test_subdir.py b/tests/test_subdir.py index 6c183c98..180bb9bc 100644 --- a/tests/test_subdir.py +++ b/tests/test_subdir.py @@ -2,28 +2,31 @@ Test if the subdirectory is correctly navigated to """ import logging -from os.path import abspath, dirname import pytest from repo2docker.app import Repo2Docker -# This is the path to the repo2docker git repository that this file exists in. -repo_path = dirname(dirname(abspath(__file__))) +TEST_REPO = "https://github.com/binderhub-ci-repos/repo2docker-subdir-support" def test_subdir(run_repo2docker): - argv = ['--subdir', 'tests/conda/simple', repo_path] + # Build from a subdirectory + # if subdir support is broken this will fail as the instructions in the + # root of the test repo are invalid + argv = ['--subdir', 'a directory', TEST_REPO] run_repo2docker(argv) def test_subdir_invalid(caplog): - caplog.set_level(logging.INFO, logger='Repo2Docker') + # test an error is raised when requesting a non existent subdir + caplog.set_level(logging.INFO) app = Repo2Docker() - argv = ['--subdir', 'tests/conda/invalid', repo_path] + argv = ['--subdir', 'invalid-sub-dir', TEST_REPO] app.initialize(argv) app.debug = True - app.run = False + # no build implies no run + app.build = False with pytest.raises(SystemExit) as excinfo: app.start() # Just build the image and do not run it. @@ -31,4 +34,4 @@ def test_subdir_invalid(caplog): assert excinfo.value.code == 1 # Can't get this to record the logs? - # assert caplog.text == "Subdirectory tests/conda/invalid does not exist" + assert caplog.text == "Subdirectory tests/conda/invalid does not exist" From df7251dff6af473d13aebccb00da62018c51079f Mon Sep 17 00:00:00 2001 From: Tim Head Date: Tue, 16 Oct 2018 10:41:06 +0200 Subject: [PATCH 2/3] Add context manager to change working dir The context manager takes care of restoring the current working directory when we are done. This is useful when the directory we set as working directory stops existing. --- repo2docker/app.py | 65 ++++++++++++++++++++++---------------------- repo2docker/utils.py | 16 +++++++++++ tests/test_subdir.py | 10 +++++-- 3 files changed, 56 insertions(+), 35 deletions(-) diff --git a/repo2docker/app.py b/repo2docker/app.py index 04f0b560..c92e9ebb 100644 --- a/repo2docker/app.py +++ b/repo2docker/app.py @@ -35,7 +35,7 @@ from .buildpacks import ( from . import contentproviders from .utils import ( ByteSpecification, maybe_cleanup, is_valid_docker_image_name, - validate_and_generate_port_mapping + validate_and_generate_port_mapping, execute_cmd, check_ref, chdir ) @@ -710,43 +710,42 @@ class Repo2Docker(Application): self.subdir, extra=dict(phase='failure')) sys.exit(1) - os.chdir(checkout_path) + with chdir(checkout_path): + for BP in self.buildpacks: + bp = BP() + if bp.detect(): + picked_buildpack = bp + break + else: + picked_buildpack = self.default_buildpack() - for BP in self.buildpacks: - bp = BP() - if bp.detect(): - picked_buildpack = bp - break - else: - picked_buildpack = self.default_buildpack() + picked_buildpack.appendix = self.appendix - picked_buildpack.appendix = self.appendix + self.log.debug(picked_buildpack.render(), + extra=dict(phase='building')) - self.log.debug(picked_buildpack.render(), - extra=dict(phase='building')) + if self.build: + build_args = { + 'NB_USER': self.user_name, + 'NB_UID': str(self.user_id) + } + self.log.info('Using %s builder\n', bp.__class__.__name__, + extra=dict(phase='building')) - if self.build: - build_args = { - 'NB_USER': self.user_name, - 'NB_UID': str(self.user_id) - } - self.log.info('Using %s builder\n', bp.__class__.__name__, - extra=dict(phase='building')) - - for l in picked_buildpack.build(self.output_image_spec, - self.build_memory_limit, build_args): - if 'stream' in l: - self.log.info(l['stream'], - extra=dict(phase='building')) - elif 'error' in l: - self.log.info(l['error'], extra=dict(phase='failure')) - sys.exit(1) - elif 'status' in l: - self.log.info('Fetching base image...\r', + for l in picked_buildpack.build(self.output_image_spec, + self.build_memory_limit, build_args): + if 'stream' in l: + self.log.info(l['stream'], + extra=dict(phase='building')) + elif 'error' in l: + self.log.info(l['error'], extra=dict(phase='failure')) + sys.exit(1) + elif 'status' in l: + self.log.info('Fetching base image...\r', + extra=dict(phase='building')) + else: + self.log.info(json.dumps(l), extra=dict(phase='building')) - else: - self.log.info(json.dumps(l), - extra=dict(phase='building')) if self.push: self.push_image() diff --git a/repo2docker/utils.py b/repo2docker/utils.py index 9d75d1cf..d6854c8f 100644 --- a/repo2docker/utils.py +++ b/repo2docker/utils.py @@ -1,5 +1,6 @@ from contextlib import contextmanager from functools import partial +import os import re import shutil import subprocess @@ -51,6 +52,21 @@ def execute_cmd(cmd, capture=False, **kwargs): raise subprocess.CalledProcessError(ret, cmd) +@contextmanager +def chdir(path): + """Change working directory to `path` and restore it again + + This context maanger is useful if `path` stops existing during your + operations. + """ + old_dir = os.getcwd() + os.chdir(path) + try: + yield + finally: + os.chdir(old_dir) + + @contextmanager def maybe_cleanup(path, cleanup=False): """Delete the directory at passed path if cleanup flag is True.""" diff --git a/tests/test_subdir.py b/tests/test_subdir.py index 180bb9bc..977f235f 100644 --- a/tests/test_subdir.py +++ b/tests/test_subdir.py @@ -1,6 +1,7 @@ """ Test if the subdirectory is correctly navigated to """ +import os import logging import pytest @@ -13,13 +14,18 @@ def test_subdir(run_repo2docker): # Build from a subdirectory # if subdir support is broken this will fail as the instructions in the # root of the test repo are invalid + cwd = os.getcwd() + argv = ['--subdir', 'a directory', TEST_REPO] run_repo2docker(argv) + # check that we restored the current working directory + assert cwd == os.getcwd(), "We should be back in %s" % cwd + def test_subdir_invalid(caplog): # test an error is raised when requesting a non existent subdir - caplog.set_level(logging.INFO) + #caplog.set_level(logging.INFO, logger='Repo2Docker') app = Repo2Docker() argv = ['--subdir', 'invalid-sub-dir', TEST_REPO] @@ -34,4 +40,4 @@ def test_subdir_invalid(caplog): assert excinfo.value.code == 1 # Can't get this to record the logs? - assert caplog.text == "Subdirectory tests/conda/invalid does not exist" + #assert caplog.text == "Subdirectory tests/conda/invalid does not exist" From b1dcbbe9964f2f88588656d849aa40bc6d27efe2 Mon Sep 17 00:00:00 2001 From: Tim Head Date: Tue, 16 Oct 2018 13:46:00 +0200 Subject: [PATCH 3/3] Adjust sub-directory tests after changing CWD behaviour --- repo2docker/app.py | 2 +- tests/test_clone_depth.py | 147 +++++++++++++++++++++----------------- tests/test_subdir.py | 3 +- 3 files changed, 86 insertions(+), 66 deletions(-) diff --git a/repo2docker/app.py b/repo2docker/app.py index c92e9ebb..17f1b39a 100644 --- a/repo2docker/app.py +++ b/repo2docker/app.py @@ -35,7 +35,7 @@ from .buildpacks import ( from . import contentproviders from .utils import ( ByteSpecification, maybe_cleanup, is_valid_docker_image_name, - validate_and_generate_port_mapping, execute_cmd, check_ref, chdir + validate_and_generate_port_mapping, chdir ) diff --git a/tests/test_clone_depth.py b/tests/test_clone_depth.py index 5067df3f..4ef8c90e 100644 --- a/tests/test_clone_depth.py +++ b/tests/test_clone_depth.py @@ -6,7 +6,11 @@ container requires a specific repository and commit to be checked out, and that is the only thing that is tested. """ +import os import subprocess + +from tempfile import TemporaryDirectory + from repo2docker.app import Repo2Docker @@ -16,88 +20,103 @@ URL = "https://github.com/binderhub-ci-repos/repo2docker-ci-clone-depth" def test_clone_depth(): """Test a remote repository, without a refspec""" - app = Repo2Docker() - argv = [URL] - app.initialize(argv) - app.debug = True - app.run = False - app.cleanup_checkout = False - app.start() # This just build the image and does not run it. + with TemporaryDirectory() as d: + app = Repo2Docker() + argv = [URL] + app.initialize(argv) + app.build = False + app.run = False + # turn of automatic clean up of the checkout so we can inspect it + # we also set the work directory explicitly so we know where to look + app.cleanup_checkout = False + app.git_workdir = d + app.start() - # Building the image has already put us in the cloned repository directory - cmd = ['git', 'rev-parse', 'HEAD'] - p = subprocess.run(cmd, stdout=subprocess.PIPE) - assert p.stdout.strip() == b'703322e9c6635ba1835d3b92eafbabeca0042c3e' - cmd = ['git', 'rev-list', '--count', 'HEAD'] - p = subprocess.run(cmd, stdout=subprocess.PIPE) - assert p.stdout.strip() == b'1' - with open('COMMIT') as fp: - assert fp.read() == '100\n' + cmd = ['git', 'rev-parse', 'HEAD'] + p = subprocess.run(cmd, stdout=subprocess.PIPE, cwd=d) + assert p.stdout.strip() == b'703322e9c6635ba1835d3b92eafbabeca0042c3e' + cmd = ['git', 'rev-list', '--count', 'HEAD'] + p = subprocess.run(cmd, stdout=subprocess.PIPE, cwd=d) + assert p.stdout.strip() == b'1' + with open(os.path.join(d, 'COMMIT')) as fp: + assert fp.read() == '100\n' def test_clone_depth_full(): """Test a remote repository, with a refspec of 'master'""" - app = Repo2Docker() - argv = ['--ref', 'master', URL] - app.initialize(argv) - app.debug = True - app.run = False - app.cleanup_checkout = False - app.start() # This just build the image and does not run it. + with TemporaryDirectory() as d: + app = Repo2Docker() + argv = ['--ref', 'master', URL] + app.initialize(argv) + app.run = False + app.build = False + # turn of automatic clean up of the checkout so we can inspect it + # we also set the work directory explicitly so we know where to look + app.cleanup_checkout = False + app.git_workdir = d + app.start() - # Building the image has already put us in the cloned repository directory - cmd = ['git', 'rev-parse', 'HEAD'] - p = subprocess.run(cmd, stdout=subprocess.PIPE) - assert p.stdout.strip() == b'703322e9c6635ba1835d3b92eafbabeca0042c3e' - cmd = ['git', 'rev-list', '--count', 'HEAD'] - p = subprocess.run(cmd, stdout=subprocess.PIPE) - assert p.stdout.strip() == b'100' - with open('COMMIT') as fp: - assert fp.read() == '100\n' + # Building the image has already put us in the cloned repository directory + cmd = ['git', 'rev-parse', 'HEAD'] + p = subprocess.run(cmd, stdout=subprocess.PIPE, cwd=d) + assert p.stdout.strip() == b'703322e9c6635ba1835d3b92eafbabeca0042c3e' + cmd = ['git', 'rev-list', '--count', 'HEAD'] + p = subprocess.run(cmd, stdout=subprocess.PIPE, cwd=d) + assert p.stdout.strip() == b'100' + with open(os.path.join(d, 'COMMIT')) as fp: + assert fp.read() == '100\n' def test_clone_depth_full2(): """Test a remote repository, with a refspec of the master commit hash""" - app = Repo2Docker() - argv = ['--ref', '703322e', URL] + with TemporaryDirectory() as d: + app = Repo2Docker() + argv = ['--ref', '703322e', URL] - app.initialize(argv) - app.debug = True - app.run = False - app.cleanup_checkout = False - app.start() # This just build the image and does not run it. + app.initialize(argv) + app.run = False + app.build = False + # turn of automatic clean up of the checkout so we can inspect it + # we also set the work directory explicitly so we know where to look + app.cleanup_checkout = False + app.git_workdir = d + app.start() - # Building the image has already put us in the cloned repository directory - cmd = ['git', 'rev-parse', 'HEAD'] - p = subprocess.run(cmd, stdout=subprocess.PIPE) - assert p.stdout.strip() == b'703322e9c6635ba1835d3b92eafbabeca0042c3e' - cmd = ['git', 'rev-list', '--count', 'HEAD'] - p = subprocess.run(cmd, stdout=subprocess.PIPE) - assert p.stdout.strip() == b'100' - with open('COMMIT') as fp: - assert fp.read() == '100\n' + # Building the image has already put us in the cloned repository directory + cmd = ['git', 'rev-parse', 'HEAD'] + p = subprocess.run(cmd, stdout=subprocess.PIPE, cwd=d) + assert p.stdout.strip() == b'703322e9c6635ba1835d3b92eafbabeca0042c3e' + cmd = ['git', 'rev-list', '--count', 'HEAD'] + p = subprocess.run(cmd, stdout=subprocess.PIPE, cwd=d) + assert p.stdout.strip() == b'100' + with open(os.path.join(d, 'COMMIT')) as fp: + assert fp.read() == '100\n' def test_clone_depth_mid(): """Test a remote repository, with a refspec of a commit hash halfway""" - app = Repo2Docker() - argv = ['--ref', '8bc4f21', URL] + with TemporaryDirectory() as d: + app = Repo2Docker() + argv = ['--ref', '8bc4f21', URL] - app.initialize(argv) - app.debug = True - app.run = False - app.cleanup_checkout = False - app.start() # This just build the image and does not run it. + app.initialize(argv) + app.run = False + app.build = False + # turn of automatic clean up of the checkout so we can inspect it + # we also set the work directory explicitly so we know where to look + app.cleanup_checkout = False + app.git_workdir = d + app.start() - # Building the image has already put us in the cloned repository directory - cmd = ['git', 'rev-parse', 'HEAD'] - p = subprocess.run(cmd, stdout=subprocess.PIPE) - assert p.stdout.strip() == b'8bc4f216856f86f6fc25a788b744b93b87e9ba48' - cmd = ['git', 'rev-list', '--count', 'HEAD'] - p = subprocess.run(cmd, stdout=subprocess.PIPE) - assert p.stdout.strip() == b'50' - with open('COMMIT') as fp: - assert fp.read() == '50\n' + # Building the image has already put us in the cloned repository directory + cmd = ['git', 'rev-parse', 'HEAD'] + p = subprocess.run(cmd, stdout=subprocess.PIPE, cwd=d) + assert p.stdout.strip() == b'8bc4f216856f86f6fc25a788b744b93b87e9ba48' + cmd = ['git', 'rev-list', '--count', 'HEAD'] + p = subprocess.run(cmd, stdout=subprocess.PIPE, cwd=d) + assert p.stdout.strip() == b'50' + with open(os.path.join(d, 'COMMIT')) as fp: + assert fp.read() == '50\n' diff --git a/tests/test_subdir.py b/tests/test_subdir.py index 977f235f..49f8e81f 100644 --- a/tests/test_subdir.py +++ b/tests/test_subdir.py @@ -31,8 +31,9 @@ def test_subdir_invalid(caplog): argv = ['--subdir', 'invalid-sub-dir', TEST_REPO] app.initialize(argv) app.debug = True - # no build implies no run + # no build does not imply no run app.build = False + app.run = False with pytest.raises(SystemExit) as excinfo: app.start() # Just build the image and do not run it.