Git: simplify submodule checks (#10635)

We are always querying for submodules,
even when the user explicitly listed the submodules to include.

We also have the option to include all submodules,
instead of listing each one of them, we can just
omit them, git will automatically default to include all of them.

Ref #10594 (comment)

Author: stsewd

readthedocs/projects/exceptions.py

@@ -39,10 +39,6 @@ class RepositoryError(BuildUserError):
         "ensure that your repository URL is correct and your repository is public. "
         "Private repositories are not supported.",
     )
-    INVALID_SUBMODULES = _(
-        "One or more submodule URLs are not valid: {}, "
-        "git/ssh URL schemas for submodules are not supported."
-    )
     DUPLICATED_RESERVED_VERSIONS = _(
         "You can not have two versions with the name latest or stable."
         " Ensure you don't have both a branch and a tag with this name."

readthedocs/projects/tests/mockers.py

@@ -149,18 +149,9 @@ def _mock_git_repository(self):
             'readthedocs.vcs_support.backends.git.Backend.submodules',
             new_callable=mock.PropertyMock,
             return_value=[
-                mock.Mock(
-                    path='one',
-                    url='https://github.com/submodule/one',
-                ),
-                mock.Mock(
-                    path='two',
-                    url='https://github.com/submodule/two',
-                ),
-                mock.Mock(
-                    path='three',
-                    url='https://github.com/submodule/three',
-                ),
+                "one",
+                "two",
+                "three",
             ],
         )
 

readthedocs/projects/tests/test_build_tasks.py

@@ -1611,7 +1611,7 @@ def test_python_install_pip_several_options(self, load_yaml_config):
     @pytest.mark.parametrize(
         "value,expected",
         [
-            (ALL, ["one", "two", "three"]),
+            (ALL, []),
             (["one", "two"], ["one", "two"]),
         ],
     )

readthedocs/projects/validators.py

@@ -79,27 +79,7 @@ def __call__(self, value):
         raise ValidationError(_("Invalid scheme for URL"))
 
 
-class SubmoduleURLValidator(RepositoryURLValidator):
-
-    """
-    A URL validator for repository submodules.
-
-    If a repository has a relative submodule, the URL path is effectively the
-    supermodule's remote ``origin`` URL with the relative path applied.
-
-    From the git docs::
-
-        ``<repository>`` is the URL of the new submodule's origin repository.
-        This may be either an absolute URL, or (if it begins with ``./`` or
-        ``../``), the location relative to the superproject's default remote
-        repository
-    """
-
-    disallow_relative_url = False
-
-
 validate_repository_url = RepositoryURLValidator()
-validate_submodule_url = SubmoduleURLValidator()
 
 
 def validate_build_config_file(path):

readthedocs/rtd_tests/tests/test_backend.py

@@ -181,24 +181,6 @@ def test_check_for_submodules(self):
         repo.checkout("submodule")
         self.assertTrue(repo.are_submodules_available(self.dummy_conf))
 
-    def test_check_invalid_submodule_urls(self):
-        """Test that a branch with an invalid submodule fails correctly."""
-        repo = self.project.vcs_repo(
-            environment=self.build_environment,
-            version_type=BRANCH,
-            version_identifier="invalidsubmodule",
-        )
-        repo.update()
-        repo.checkout("invalidsubmodule")
-        with self.assertRaises(RepositoryError) as e:
-            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"]),
-        )
-
     def test_check_submodule_urls(self):
         """Test that a valid submodule is found in the 'submodule' branch."""
         repo = self.project.vcs_repo(
@@ -208,7 +190,7 @@ def test_check_submodule_urls(self):
         )
         repo.update()
         repo.checkout("submodule")
-        valid, _ = repo.validate_submodules(self.dummy_conf)
+        valid, _ = repo.get_available_submodules(self.dummy_conf)
         self.assertTrue(valid)
 
     @patch("readthedocs.vcs_support.backends.git.Backend.fetch")
@@ -231,7 +213,7 @@ def test_git_update_with_external_version(self, fetch):
         repo.update()
         fetch.assert_called_once()
 
-    def test_invalid_submodule_is_ignored(self):
+    def test_submodule_without_url_is_included(self):
         """Test that an invalid submodule isn't listed."""
         repo = self.project.vcs_repo(
             environment=self.build_environment,
@@ -252,9 +234,8 @@ def test_invalid_submodule_is_ignored(self):
             )
             f.write(content)
 
-        valid, submodules = repo.validate_submodules(self.dummy_conf)
-        self.assertTrue(valid)
-        self.assertEqual(list(submodules), ["foobar"])
+        submodules = list(repo.submodules)
+        self.assertEqual(submodules, ["foobar", "not-valid-path"])
 
     def test_parse_submodules(self):
         repo = self.project.vcs_repo(
@@ -296,10 +277,16 @@ def test_parse_submodules(self):
             )
             f.write(content)
 
-        valid, submodules = repo.validate_submodules(self.dummy_conf)
-        self.assertTrue(valid)
+        submodules = list(repo.submodules)
         self.assertEqual(
-            list(submodules), ["foobar", "path with spaces", "another-submodule"]
+            submodules,
+            [
+                "foobar",
+                "not-valid-path",
+                "path with spaces",
+                "another-submodule",
+                "invalid-submodule-key",
+            ],
         )
 
     def test_skip_submodule_checkout(self):