From 2394d2812803186dbb234542a109f7e25f9f5940 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 17 Dec 2018 16:51:29 -0500 Subject: [PATCH 1/3] Return invalid submodules too --- readthedocs/projects/exceptions.py | 2 +- readthedocs/vcs_support/backends/git.py | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/readthedocs/projects/exceptions.py b/readthedocs/projects/exceptions.py index ba4196320a1..41daacdeaaf 100644 --- a/readthedocs/projects/exceptions.py +++ b/readthedocs/projects/exceptions.py @@ -40,7 +40,7 @@ class RepositoryError(BuildEnvironmentError): ) INVALID_SUBMODULES = _( - 'One or more submodule URLs are not valid.' + 'One or more submodule URLs are not valid: {}.' ) DUPLICATED_RESERVED_VERSIONS = _( diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 6c6e3077dbd..b37c2fd2500 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -122,11 +122,15 @@ def validate_submodules(self, config): submodules_include[path] = submodules[path] submodules = submodules_include + invalid_submodules = [] for path, submodule in submodules.items(): try: validate_submodule_url(submodule.url) except ValidationError: - return False, [] + invalid_submodules.append(path) + + if invalid_submodules: + return False, invalid_submodules return True, submodules.keys() def fetch(self): @@ -222,7 +226,9 @@ def update_submodules(self, config): if valid: self.checkout_submodules(submodules, config) else: - raise RepositoryError(RepositoryError.INVALID_SUBMODULES) + raise RepositoryError( + RepositoryError.INVALID_SUBMODULES.format(submodules) + ) def checkout_submodules(self, submodules, config): """Checkout all repository submodules.""" From d8c2131c13712cec7138a8dcff6c66ff2f80594c Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 17 Dec 2018 16:53:40 -0500 Subject: [PATCH 2/3] Refactor test --- readthedocs/rtd_tests/tests/test_backend.py | 24 ++++---- readthedocs/rtd_tests/utils.py | 63 +++++++++++++-------- 2 files changed, 52 insertions(+), 35 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_backend.py b/readthedocs/rtd_tests/tests/test_backend.py index ee3471528ca..46f1a80b406 100644 --- a/readthedocs/rtd_tests/tests/test_backend.py +++ b/readthedocs/rtd_tests/tests/test_backend.py @@ -56,7 +56,6 @@ def test_git_branches(self, checkout_path): default_branches = [ # comes from ``make_test_git`` function 'submodule', - 'relativesubmodule', 'invalidsubmodule', ] branches = [ @@ -89,7 +88,6 @@ def test_git_branches_unicode(self, checkout_path): default_branches = [ # comes from ``make_test_git`` function 'submodule', - 'relativesubmodule', 'invalidsubmodule', ] branches = [ @@ -163,15 +161,19 @@ def test_check_submodule_urls(self): repo.checkout('submodule') valid, _ = repo.validate_submodules(self.dummy_conf) self.assertTrue(valid) - repo.checkout('relativesubmodule') - valid, _ = repo.validate_submodules(self.dummy_conf) - self.assertTrue(valid) - @pytest.mark.xfail(strict=True, reason="Fixture is not working correctly") def test_check_invalid_submodule_urls(self): + repo = self.project.vcs_repo() + repo.update() + r = repo.checkout('invalidsubmodule') with self.assertRaises(RepositoryError) as e: - repo.checkout('invalidsubmodule') - self.assertEqual(e.msg, RepositoryError.INVALID_SUBMODULES) + repo.update_submodules(self.dummy_conf) + # `invalid` is created in `make_test_git` + # it's a url in ssh form. + self.assertEqual( + str(e.exception), + RepositoryError.INVALID_SUBMODULES.format(['invalid']) + ) @patch('readthedocs.projects.models.Project.checkout_path') def test_fetch_clean_tags_and_branches(self, checkout_path): @@ -197,8 +199,7 @@ def test_fetch_clean_tags_and_branches(self, checkout_path): ) self.assertEqual( set([ - 'relativesubmodule', 'invalidsubmodule', - 'master', 'submodule', 'newbranch', + 'invalidsubmodule', 'master', 'submodule', 'newbranch', ]), set(vcs.verbose_name for vcs in repo.branches) ) @@ -212,8 +213,7 @@ def test_fetch_clean_tags_and_branches(self, checkout_path): ) self.assertEqual( set([ - 'relativesubmodule', 'invalidsubmodule', - 'master', 'submodule' + 'invalidsubmodule', 'master', 'submodule' ]), set(vcs.verbose_name for vcs in repo.branches) ) diff --git a/readthedocs/rtd_tests/utils.py b/readthedocs/rtd_tests/utils.py index 7ad6bed10cd..d953ec76423 100644 --- a/readthedocs/rtd_tests/utils.py +++ b/readthedocs/rtd_tests/utils.py @@ -2,10 +2,15 @@ """Utility functions for use in tests.""" from __future__ import ( - absolute_import, division, print_function, unicode_literals) + absolute_import, + division, + print_function, + unicode_literals, +) import logging import subprocess +import textwrap from os import chdir, environ, mkdir from os.path import abspath from os.path import join as pjoin @@ -55,34 +60,16 @@ def make_test_git(): # URL are not allowed and using a real URL will require Internet to clone # the repo check_output(['git', 'checkout', '-b', 'submodule', 'master'], env=env) - # https://stackoverflow.com/a/37378302/2187091 - mkdir(pjoin(directory, 'foobar')) - gitmodules_path = pjoin(directory, '.gitmodules') - with open(gitmodules_path, 'w') as fh: - fh.write('''[submodule "foobar"]\n\tpath = foobar\n\turl = https://foobar.com/git\n''') - check_output( - [ - 'git', 'update-index', '--add', '--cacheinfo', '160000', - '233febf4846d7a0aeb95b6c28962e06e21d13688', 'foobar', - ], - env=env, + add_submodule_without_cloning( + directory, 'foobar', 'https://foobar.com/git' ) check_output(['git', 'add', '.'], env=env) check_output(['git', 'commit', '-m"Add submodule"'], env=env) - # Add a relative submodule URL in the relativesubmodule branch - check_output(['git', 'checkout', '-b', 'relativesubmodule', 'master'], env=env) - check_output( - ['git', 'submodule', 'add', '-b', 'master', './', 'relativesubmodule'], - env=env - ) - check_output(['git', 'add', '.'], env=env) - check_output(['git', 'commit', '-m"Add relative submodule"'], env=env) # Add an invalid submodule URL in the invalidsubmodule branch check_output(['git', 'checkout', '-b', 'invalidsubmodule', 'master'], env=env) - check_output( - ['git', 'submodule', 'add', '-b', 'master', './', 'invalidsubmodule'], - env=env, + add_submodule_without_cloning( + directory, 'invalid', 'git@github.com:rtfd/readthedocs.org.git' ) check_output(['git', 'add', '.'], env=env) check_output(['git', 'commit', '-m"Add invalid submodule"'], env=env) @@ -92,6 +79,36 @@ def make_test_git(): return directory +@restoring_chdir +def add_submodule_without_cloning(directory, submodule, url): + """ + Add a submodule without cloning it. + + We write directly to the git index, more details in: + https://stackoverflow.com/a/37378302/2187091 + """ + env = environ.copy() + env['GIT_DIR'] = pjoin(directory, '.git') + chdir(directory) + + mkdir(pjoin(directory, submodule)) + gitmodules_path = pjoin(directory, '.gitmodules') + with open(gitmodules_path, 'w+') as fh: + content = textwrap.dedent(''' + [submodule "{submodule}"] + path = {submodule} + url = {url} + ''') + fh.write(content.format(submodule=submodule, url=url)) + check_output( + [ + 'git', 'update-index', '--add', '--cacheinfo', '160000', + '233febf4846d7a0aeb95b6c28962e06e21d13688', submodule, + ], + env=env, + ) + + @restoring_chdir def make_git_repo(directory, name='sample_repo'): path = get_readthedocs_app_path() From 297aa2599cfdab061e9c4a05c4ce08d1cfcc1210 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 18 Dec 2018 10:40:15 -0500 Subject: [PATCH 3/3] Update docstrings --- readthedocs/rtd_tests/utils.py | 13 ++++++++++--- readthedocs/vcs_support/backends/git.py | 5 ++++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/readthedocs/rtd_tests/utils.py b/readthedocs/rtd_tests/utils.py index d953ec76423..1908ef0ccd5 100644 --- a/readthedocs/rtd_tests/utils.py +++ b/readthedocs/rtd_tests/utils.py @@ -60,7 +60,7 @@ def make_test_git(): # URL are not allowed and using a real URL will require Internet to clone # the repo check_output(['git', 'checkout', '-b', 'submodule', 'master'], env=env) - add_submodule_without_cloning( + add_git_submodule_without_cloning( directory, 'foobar', 'https://foobar.com/git' ) check_output(['git', 'add', '.'], env=env) @@ -68,7 +68,7 @@ def make_test_git(): # Add an invalid submodule URL in the invalidsubmodule branch check_output(['git', 'checkout', '-b', 'invalidsubmodule', 'master'], env=env) - add_submodule_without_cloning( + add_git_submodule_without_cloning( directory, 'invalid', 'git@github.com:rtfd/readthedocs.org.git' ) check_output(['git', 'add', '.'], env=env) @@ -80,12 +80,19 @@ def make_test_git(): @restoring_chdir -def add_submodule_without_cloning(directory, submodule, url): +def add_git_submodule_without_cloning(directory, submodule, url): """ Add a submodule without cloning it. We write directly to the git index, more details in: https://stackoverflow.com/a/37378302/2187091 + + :param directory: The directory where the git repo is + :type directory: str + :param submodule: The name of the submodule to be created + :type submodule: str + :param url: The url where the submodule points to + :type url: str """ env = environ.copy() env['GIT_DIR'] = pjoin(directory, '.git') diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index b37c2fd2500..567634e82a6 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -96,13 +96,16 @@ def validate_submodules(self, config): :returns: tuple(bool, list) - Returns true if all required submodules URLs are valid. + Returns `True` if all required submodules URLs are valid. Returns a list of all required submodules: - Include is `ALL`, returns all submodules avaliable. - Include is a list, returns just those. - Exclude is `ALL` - this should never happen. - Exlude is a list, returns all avaliable submodules but those from the list. + + Returns `False` if at least one submodule is invalid. + Returns the list of invalid submodules. """ repo = git.Repo(self.working_dir) submodules = {