ref(dep): Addressing dependency edge cases (#44)

* ref(dep): Addressing dependency edge cases

* Add extra context to dependency error

* Fix circular import

devservices/exceptions.py

@@ -39,3 +39,12 @@ def __init__(self, command: str, returncode: int, stdout: str, stderr: str):
         self.returncode = returncode
         self.stdout = stdout
         self.stderr = stderr
+
+
+class DependencyError(Exception):
+    """Base class for dependency-related errors."""
+
+    def __init__(self, repo_name: str, repo_link: str, branch: str):
+        self.repo_name = repo_name
+        self.repo_link = repo_link
+        self.branch = branch

devservices/utils/dependencies.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import os
+import shutil
 import subprocess
 from concurrent.futures import as_completed
 from concurrent.futures import ThreadPoolExecutor
@@ -11,6 +12,7 @@
 from devservices.configs.service_config import RemoteConfig
 from devservices.constants import DEVSERVICES_DIR_NAME
 from devservices.constants import DEVSERVICES_LOCAL_DEPENDENCIES_DIR
+from devservices.exceptions import DependencyError
 
 
 def install_dependencies(dependencies: list[Dependency]) -> None:
@@ -43,8 +45,7 @@ def install_dependency(dependency: RemoteConfig) -> None:
         DEVSERVICES_LOCAL_DEPENDENCIES_DIR, dependency.repo_name
     )
 
-    # TODO: make this smarter (check if a valid repo)
-    if os.path.exists(dependency_repo_dir):
+    if os.path.exists(dependency_repo_dir) and _is_valid_repo(dependency_repo_dir):
         _update_dependency(dependency, dependency_repo_dir)
     else:
         _checkout_dependency(dependency, dependency_repo_dir)
@@ -54,10 +55,17 @@ def _update_dependency(
     dependency: RemoteConfig,
     dependency_repo_dir: str,
 ) -> None:
-    _run_command(
-        ["git", "fetch", "origin", dependency.branch, "--filter=blob:none"],
-        dependency_repo_dir,
-    )
+    try:
+        _run_command(
+            ["git", "fetch", "origin", dependency.branch, "--filter=blob:none"],
+            cwd=dependency_repo_dir,
+        )
+    except subprocess.CalledProcessError:
+        raise DependencyError(
+            repo_name=dependency.repo_name,
+            repo_link=dependency.repo_link,
+            branch=dependency.branch,
+        )
 
     # Check if the local repo is up-to-date
     local_commit = subprocess.check_output(
@@ -76,16 +84,17 @@ def _update_dependency(
         # Already up-to-date, don't pull anything
         return
 
-    # If it's not up-to-date, checkout the latest changes
-    _run_command(["git", "checkout", "FETCH_HEAD"], cwd=dependency_repo_dir)
+    # If it's not up-to-date, checkout the latest changes (forcibly)
+    _run_command(["git", "checkout", "-f", "FETCH_HEAD"], cwd=dependency_repo_dir)
 
 
 def _checkout_dependency(
     dependency: RemoteConfig,
     dependency_repo_dir: str,
 ) -> None:
-    # TODO: Should we clean up the directory if it already exists?
-    os.makedirs(dependency_repo_dir, exist_ok=True)
+    if os.path.exists(dependency_repo_dir):
+        shutil.rmtree(dependency_repo_dir)
+    os.makedirs(dependency_repo_dir, exist_ok=False)
 
     _run_command(
         [
@@ -122,6 +131,16 @@ def _checkout_dependency(
     )
 
 
+def _is_valid_repo(path: str) -> bool:
+    if not os.path.exists(os.path.join(path, ".git")):
+        return False
+    try:
+        _run_command(["git", "rev-parse", "--is-inside-work-tree"], cwd=path)
+        return True
+    except subprocess.CalledProcessError:
+        return False
+
+
 def _has_remote_config(remote_config: RemoteConfig | None) -> TypeGuard[RemoteConfig]:
     return remote_config is not None
 

tests/utils/test_dependencies.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import shutil
 from pathlib import Path
 from subprocess import SubprocessError
 from unittest import mock
@@ -9,6 +10,7 @@
 from devservices.configs.service_config import RemoteConfig
 from devservices.constants import CONFIG_FILE_NAME
 from devservices.constants import DEVSERVICES_DIR_NAME
+from devservices.exceptions import DependencyError
 from devservices.utils.dependencies import install_dependency
 from testing.utils import create_mock_git_repo
 from testing.utils import run_git_command
@@ -182,3 +184,167 @@ def test_install_dependency_basic_with_new_tracked_file(tmp_path: Path) -> None:
             / DEVSERVICES_DIR_NAME
             / "new-file.txt"
         ).exists()
+
+
+def test_install_dependency_basic_with_existing_dir(tmp_path: Path) -> None:
+    with mock.patch(
+        "devservices.utils.dependencies.DEVSERVICES_LOCAL_DEPENDENCIES_DIR",
+        str(tmp_path / "dependency-dir"),
+    ):
+        create_mock_git_repo("basic_repo", tmp_path / "test-repo")
+        mock_dependency = RemoteConfig(
+            repo_name="test-repo",
+            branch="main",
+            repo_link=f"file://{tmp_path / 'test-repo'}",
+        )
+
+        # Create the dependency directory and populate it
+        dependency_dir = tmp_path / "dependency-dir" / "test-repo"
+        dependency_dir.mkdir(parents=True, exist_ok=True)
+        (dependency_dir / "existing-file.txt").touch()
+
+        install_dependency(mock_dependency)
+
+        # Make sure that files outside of the devservices directory are not copied
+        assert not (tmp_path / "dependency-dir" / "test-repo" / "README.md").exists()
+
+        assert (
+            tmp_path
+            / "dependency-dir"
+            / "test-repo"
+            / DEVSERVICES_DIR_NAME
+            / CONFIG_FILE_NAME
+        ).exists()
+
+
+def test_install_dependency_basic_with_existing_invalid_repo(tmp_path: Path) -> None:
+    with mock.patch(
+        "devservices.utils.dependencies.DEVSERVICES_LOCAL_DEPENDENCIES_DIR",
+        str(tmp_path / "dependency-dir"),
+    ):
+        create_mock_git_repo("basic_repo", tmp_path / "test-repo")
+        mock_dependency = RemoteConfig(
+            repo_name="test-repo",
+            branch="main",
+            repo_link=f"file://{tmp_path / 'test-repo'}",
+        )
+
+        # Create the dependency directory and populate it
+        dependency_dir = tmp_path / "dependency-dir" / "test-repo"
+        dependency_dir.mkdir(parents=True, exist_ok=True)
+        dependency_git_dir = dependency_dir / ".git"
+        dependency_git_dir.mkdir(parents=True, exist_ok=True)
+        (dependency_dir / "existing-file.txt").touch()
+
+        install_dependency(mock_dependency)
+
+        # Make sure that files outside of the devservices directory are not copied
+        assert not (tmp_path / "dependency-dir" / "test-repo" / "README.md").exists()
+
+        assert (
+            tmp_path
+            / "dependency-dir"
+            / "test-repo"
+            / DEVSERVICES_DIR_NAME
+            / CONFIG_FILE_NAME
+        ).exists()
+
+
+def test_install_dependency_basic_with_existing_repo_conflicts(tmp_path: Path) -> None:
+    with mock.patch(
+        "devservices.utils.dependencies.DEVSERVICES_LOCAL_DEPENDENCIES_DIR",
+        str(tmp_path / "dependency-dir"),
+    ):
+        mock_git_repo = create_mock_git_repo("basic_repo", tmp_path / "test-repo")
+        mock_dependency = RemoteConfig(
+            repo_name="test-repo",
+            branch="main",
+            repo_link=f"file://{tmp_path / 'test-repo'}",
+        )
+
+        install_dependency(mock_dependency)
+
+        # Make sure that files outside of the devservices directory are not copied
+        assert not (tmp_path / "dependency-dir" / "test-repo" / "README.md").exists()
+
+        assert (
+            tmp_path
+            / "dependency-dir"
+            / "test-repo"
+            / DEVSERVICES_DIR_NAME
+            / CONFIG_FILE_NAME
+        ).exists()
+
+        # Append a new line to the config file in the mock repo and commit the change
+        with open(
+            mock_git_repo / DEVSERVICES_DIR_NAME / CONFIG_FILE_NAME, mode="a"
+        ) as f:
+            f.write("\nEdited config file")
+
+        run_git_command(["add", "."], cwd=mock_git_repo)
+        run_git_command(["commit", "-m", "Edit config file"], cwd=mock_git_repo)
+
+        # Edit the working copy and leave changes unstaged
+        with open(
+            tmp_path
+            / "dependency-dir"
+            / "test-repo"
+            / DEVSERVICES_DIR_NAME
+            / CONFIG_FILE_NAME,
+            mode="a",
+        ) as f:
+            f.write("\nConflict")
+
+        install_dependency(mock_dependency)
+
+        # Check that the config file in the dependency directory has the new line appended
+        with open(
+            tmp_path
+            / "dependency-dir"
+            / "test-repo"
+            / DEVSERVICES_DIR_NAME
+            / CONFIG_FILE_NAME,
+            mode="r",
+        ) as f:
+            assert f.read().endswith("\nEdited config file")
+
+
+def test_install_dependency_basic_with_corrupted_repo(tmp_path: Path) -> None:
+    with mock.patch(
+        "devservices.utils.dependencies.DEVSERVICES_LOCAL_DEPENDENCIES_DIR",
+        str(tmp_path / "dependency-dir"),
+    ):
+        mock_git_repo = create_mock_git_repo("basic_repo", tmp_path / "test-repo")
+        mock_dependency = RemoteConfig(
+            repo_name="test-repo",
+            branch="main",
+            repo_link=f"file://{tmp_path / 'test-repo'}",
+        )
+
+        # Sanity check that the config file is not in the dependency directory (yet)
+        assert not (
+            tmp_path
+            / "dependency-dir"
+            / "test-repo"
+            / DEVSERVICES_DIR_NAME
+            / CONFIG_FILE_NAME
+        ).exists()
+
+        install_dependency(mock_dependency)
+
+        # Sanity check that the new file is not in the dependency directory (yet)
+        assert not (tmp_path / "dependency-dir" / "test-repo" / "new-file.txt").exists()
+
+        assert (
+            tmp_path
+            / "dependency-dir"
+            / "test-repo"
+            / DEVSERVICES_DIR_NAME
+            / CONFIG_FILE_NAME
+        ).exists()
+
+        # Corrupt the git repository by deleting the .git directory
+        shutil.rmtree(mock_git_repo / ".git")
+
+        with pytest.raises(DependencyError):
+            install_dependency(mock_dependency)