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/rtd_tests/tests/test_backend.py b/readthedocs/rtd_tests/tests/test_backend.py index 700d17bd4ed..ca0de802c9e 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 = [ @@ -176,15 +174,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): @@ -210,8 +212,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) ) @@ -225,8 +226,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..1908ef0ccd5 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_git_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_git_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,43 @@ def make_test_git(): return directory +@restoring_chdir +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') + 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() diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 0e140d4db14..f9608799570 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -98,13 +98,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 = { @@ -124,11 +127,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 use_shallow_clone(self): @@ -243,7 +250,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."""