Implement submodules key from v2 config (#4493)

stsewd · agjohnson · commit bfa6bdb3460a · 2018-10-02T10:09:10.000-06:00
* Test conf.py default value

* Break submodules operation

* Use the config object to update submodules

* Introduce a dummy object in tests

* Linter

* Add test for submodules v2

* Split call

* Fix env

* Fix boolean operator

* Set v1 compatible with submodules key

* Fix tests

* Whitespace

* Unskip tests

* Fix merge

* Test correct defaults

* Fix test

* Docstrings

* One more test
diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py
@@ -549,6 +549,14 @@ def mkdocs(self):
             fail_on_warning=False,
         )
 
+    @property
+    def submodules(self):
+        return Submodules(
+            include=ALL,
+            exclude=[],
+            recursive=True,
+        )
+
 
 class BuildConfigV2(BuildConfigBase):
 
@@ -871,7 +879,8 @@ def validate_submodules(self):
             submodules['include'] = include
 
         with self.catch_validation_error('submodules.exclude'):
-            exclude = raw_submodules.get('exclude', [])
+            default = [] if submodules['include'] else ALL
+            exclude = raw_submodules.get('exclude', default)
             if exclude != ALL:
                 exclude = [
                     validate_string(submodule)
@@ -880,7 +889,11 @@ def validate_submodules(self):
             submodules['exclude'] = exclude
 
         with self.catch_validation_error('submodules'):
-            if submodules['exclude'] and submodules['include']:
+            is_including = bool(submodules['include'])
+            is_excluding = (
+                submodules['exclude'] == ALL or bool(submodules['exclude'])
+            )
+            if is_including and is_excluding:
                 self.error(
                     'submodules',
                     'You can not exclude and include submodules '
diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py
@@ -1552,6 +1552,13 @@ def test_validates_different_filetype_sphinx(self):
         assert 'configured as "Sphinx HtmlDir"' in str(excinfo.value)
         assert 'your "sphinx.builder" key does not match' in str(excinfo.value)
 
+    def test_submodule_defaults(self):
+        build = self.get_build_config({})
+        build.validate()
+        assert build.submodules.include == []
+        assert build.submodules.exclude == ALL
+        assert build.submodules.recursive is False
+
     @pytest.mark.parametrize('value', [[], 'invalid', 0])
     def test_submodules_check_invalid_type(self, value):
         build = self.get_build_config({'submodules': value})
@@ -1682,7 +1689,7 @@ def test_submodules_recursive_explict_default(self):
         })
         build.validate()
         assert build.submodules.include == []
-        assert build.submodules.exclude == []
+        assert build.submodules.exclude == ALL
         assert build.submodules.recursive is False
 
         build = self.get_build_config({
diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py
@@ -115,13 +115,7 @@ def sync_repo(self):
                         identifier=self.version.identifier,
                     ),
                 )
-                version_repo = self.project.vcs_repo(
-                    self.version.slug,
-                    # When called from ``SyncRepositoryTask.run`` we don't have
-                    # a ``setup_env`` so we use just ``None`` and commands won't
-                    # be recorded
-                    getattr(self, 'setup_env', None),
-                )
+                version_repo = self.get_vcs_repo()
                 version_repo.checkout(self.version.identifier)
             finally:
                 after_vcs.send(sender=self.version)
@@ -432,6 +426,8 @@ def run_setup(self, record=True):
                         exception=str(e),
                     ))
 
+            self.additional_vcs_operations()
+
         if self.setup_env.failure or self.config is None:
             self._log('Failing build because of setup failure: %s' % self.setup_env.failure)
 
@@ -448,6 +444,27 @@ def run_setup(self, record=True):
 
         return True
 
+    def additional_vcs_operations(self):
+        """
+        Execution of tasks that involve the project's VCS.
+
+        All this tasks have access to the configuration object.
+        """
+        version_repo = self.get_vcs_repo()
+        if version_repo.supports_submodules:
+            version_repo.update_submodules(self.config)
+
+    def get_vcs_repo(self):
+        """Get the VCS object of the current project."""
+        version_repo = self.project.vcs_repo(
+            self.version.slug,
+            # When called from ``SyncRepositoryTask.run`` we don't have
+            # a ``setup_env`` so we use just ``None`` and commands won't
+            # be recorded
+            getattr(self, 'setup_env', None),
+        )
+        return version_repo
+
     def run_build(self, docker, record):
         """
         Build the docs in an environment.
diff --git a/readthedocs/rtd_tests/tests/test_backend.py b/readthedocs/rtd_tests/tests/test_backend.py
@@ -1,17 +1,21 @@
 # -*- coding: utf-8 -*-
 
-from __future__ import absolute_import, unicode_literals
+from __future__ import (
+    absolute_import, division, print_function, unicode_literals)
+
 from os.path import exists
 
+import django_dynamic_fixture as fixture
 import pytest
 from django.contrib.auth.models import User
-import django_dynamic_fixture as fixture
+from mock import Mock
 
+from readthedocs.config import ALL
 from readthedocs.projects.exceptions import RepositoryError
-from readthedocs.projects.models import Project, Feature
+from readthedocs.projects.models import Feature, Project
 from readthedocs.rtd_tests.base import RTDTestCase
-
-from readthedocs.rtd_tests.utils import create_git_tag, make_test_git, make_test_hg
+from readthedocs.rtd_tests.utils import (
+    create_git_tag, make_test_git, make_test_hg)
 
 
 class TestGitBackend(RTDTestCase):
@@ -28,6 +32,10 @@ def setUp(self):
             repo=git_repo
         )
         self.project.users.add(self.eric)
+        self.dummy_conf = Mock()
+        # These are the default values from v1
+        self.dummy_conf.submodules.include = ALL
+        self.dummy_conf.submodules.exclude = []
 
     def test_parse_branches(self):
         data = """
@@ -77,30 +85,32 @@ def test_check_for_submodules(self):
         repo = self.project.vcs_repo()
 
         repo.checkout()
-        self.assertFalse(repo.are_submodules_available())
+        self.assertFalse(repo.are_submodules_available(self.dummy_conf))
 
         # The submodule branch contains one submodule
         repo.checkout('submodule')
-        self.assertTrue(repo.are_submodules_available())
+        self.assertTrue(repo.are_submodules_available(self.dummy_conf))
 
     def test_skip_submodule_checkout(self):
         repo = self.project.vcs_repo()
         repo.checkout('submodule')
-        self.assertTrue(repo.are_submodules_available())
+        self.assertTrue(repo.are_submodules_available(self.dummy_conf))
         feature = fixture.get(
             Feature,
             projects=[self.project],
             feature_id=Feature.SKIP_SUBMODULES,
         )
         self.assertTrue(self.project.has_feature(Feature.SKIP_SUBMODULES))
-        self.assertFalse(repo.are_submodules_available())
+        self.assertFalse(repo.are_submodules_available(self.dummy_conf))
 
     def test_check_submodule_urls(self):
         repo = self.project.vcs_repo()
         repo.checkout('submodule')
-        self.assertTrue(repo.are_submodules_valid())
+        valid, _ = repo.validate_submodules(self.dummy_conf)
+        self.assertTrue(valid)
         repo.checkout('relativesubmodule')
-        self.assertTrue(repo.are_submodules_valid())
+        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):
diff --git a/readthedocs/rtd_tests/tests/test_config_integration.py b/readthedocs/rtd_tests/tests/test_config_integration.py
@@ -13,13 +13,14 @@
 from mock import MagicMock, PropertyMock, patch
 
 from readthedocs.builds.models import Version
-from readthedocs.config import BuildConfigV1, InvalidConfig, ProjectConfig
+from readthedocs.config import ALL, BuildConfigV1, InvalidConfig, ProjectConfig
 from readthedocs.config.tests.utils import apply_fs
 from readthedocs.doc_builder.config import load_yaml_config
 from readthedocs.doc_builder.environments import LocalBuildEnvironment
 from readthedocs.doc_builder.python_environments import Conda, Virtualenv
 from readthedocs.projects import tasks
 from readthedocs.projects.models import Feature, Project
+from readthedocs.rtd_tests.utils import create_git_submodule, make_git_repo
 
 
 def create_load(config=None):
@@ -889,3 +890,104 @@ def test_mkdocs_fail_on_warning(
         assert '--strict' in args
         append_conf.assert_called_once()
         move.assert_called_once()
+
+    @pytest.mark.parametrize('value,expected', [(ALL, ['one', 'two', 'three']),
+                                                (['one', 'two'], ['one', 'two'])])
+    @patch('readthedocs.vcs_support.backends.git.Backend.checkout_submodules')
+    def test_submodules_include(self, checkout_submodules,
+                                checkout_path, tmpdir, value, expected):
+        checkout_path.return_value = str(tmpdir)
+        self.create_config_file(
+            tmpdir,
+            {
+                'submodules': {
+                    'include': value,
+                },
+            }
+        )
+
+        git_repo = make_git_repo(str(tmpdir))
+        create_git_submodule(git_repo, 'one')
+        create_git_submodule(git_repo, 'two')
+        create_git_submodule(git_repo, 'three')
+
+        update_docs = self.get_update_docs_task()
+        checkout_path.return_value = git_repo
+        update_docs.additional_vcs_operations()
+
+        args, kwargs = checkout_submodules.call_args
+        assert set(args[0]) == set(expected)
+        assert update_docs.config.submodules.recursive is False
+
+    @patch('readthedocs.vcs_support.backends.git.Backend.checkout_submodules')
+    def test_submodules_exclude(self, checkout_submodules,
+                                checkout_path, tmpdir):
+        checkout_path.return_value = str(tmpdir)
+        self.create_config_file(
+            tmpdir,
+            {
+                'submodules': {
+                    'exclude': ['one'],
+                    'recursive': True,
+                },
+            }
+        )
+
+        git_repo = make_git_repo(str(tmpdir))
+        create_git_submodule(git_repo, 'one')
+        create_git_submodule(git_repo, 'two')
+        create_git_submodule(git_repo, 'three')
+
+        update_docs = self.get_update_docs_task()
+        checkout_path.return_value = git_repo
+        update_docs.additional_vcs_operations()
+
+        args, kwargs = checkout_submodules.call_args
+        assert set(args[0]) == {'two', 'three'}
+        assert update_docs.config.submodules.recursive is True
+
+    @patch('readthedocs.vcs_support.backends.git.Backend.checkout_submodules')
+    def test_submodules_exclude_all(self, checkout_submodules,
+                                    checkout_path, tmpdir):
+        checkout_path.return_value = str(tmpdir)
+        self.create_config_file(
+            tmpdir,
+            {
+                'submodules': {
+                    'exclude': ALL,
+                    'recursive': True,
+                },
+            }
+        )
+
+        git_repo = make_git_repo(str(tmpdir))
+        create_git_submodule(git_repo, 'one')
+        create_git_submodule(git_repo, 'two')
+        create_git_submodule(git_repo, 'three')
+
+        update_docs = self.get_update_docs_task()
+        checkout_path.return_value = git_repo
+        update_docs.additional_vcs_operations()
+
+        checkout_submodules.assert_not_called()
+
+    @patch('readthedocs.vcs_support.backends.git.Backend.checkout_submodules')
+    def test_submodules_default_exclude_all(self, checkout_submodules,
+                                            checkout_path, tmpdir):
+
+        checkout_path.return_value = str(tmpdir)
+        self.create_config_file(
+            tmpdir,
+            {}
+        )
+
+        git_repo = make_git_repo(str(tmpdir))
+        create_git_submodule(git_repo, 'one')
+        create_git_submodule(git_repo, 'two')
+        create_git_submodule(git_repo, 'three')
+
+        update_docs = self.get_update_docs_task()
+        checkout_path.return_value = git_repo
+        update_docs.additional_vcs_operations()
+
+        checkout_submodules.assert_not_called()
diff --git a/readthedocs/rtd_tests/tests/test_doc_builder.py b/readthedocs/rtd_tests/tests/test_doc_builder.py
@@ -63,7 +63,7 @@ def test_create_conf_py(
         having a ``TypeError`` because of an encoding problem in Python3)
         """
         tmp_dir = tempfile.mkdtemp()
-        checkout_path.return_value = tmp_dir 
+        checkout_path.return_value = tmp_dir
         docs_dir.return_value = tmp_dir
         create_index.return_value = 'README.rst'
         get_config_params.return_value = {}
@@ -191,7 +191,6 @@ def test_get_theme_name(self, checkout_path):
         }
         self.assertEqual(builder.get_theme_name(config), 'mydir')
 
-
     @patch('readthedocs.doc_builder.base.BaseBuilder.run')
     @patch('readthedocs.projects.models.Project.checkout_path')
     def test_append_conf_create_yaml(self, checkout_path, run):
diff --git a/readthedocs/rtd_tests/utils.py b/readthedocs/rtd_tests/utils.py
diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py
diff --git a/readthedocs/vcs_support/base.py b/readthedocs/vcs_support/base.py
