From dd0b622826c4e4020f464d5897adb759007ea5f4 Mon Sep 17 00:00:00 2001 From: Vini Brasil Date: Wed, 2 Oct 2024 11:46:18 -0300 Subject: [PATCH] Add Git validations for publishing tools (#1381) This commit prevents tools from being published if the underlying Git repository is unsynced with origin. --- poetry.lock | 27 +++++-- pyproject.toml | 1 + src/crewai/cli/deploy/main.py | 13 ++-- src/crewai/cli/git.py | 80 +++++++++++++++++++++ src/crewai/cli/tools/main.py | 12 ++++ src/crewai/cli/utils.py | 34 --------- tests/cli/deploy/test_deploy_main.py | 6 +- tests/cli/test_git.py | 101 +++++++++++++++++++++++++++ tests/cli/tools/test_main.py | 14 ++-- 9 files changed, 235 insertions(+), 53 deletions(-) create mode 100644 src/crewai/cli/git.py create mode 100644 tests/cli/test_git.py diff --git a/poetry.lock b/poetry.lock index c2db023dd..753bbd8ca 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1581,12 +1581,12 @@ files = [ google-auth = ">=2.14.1,<3.0.dev0" googleapis-common-protos = ">=1.56.2,<2.0.dev0" grpcio = [ - {version = ">=1.49.1,<2.0dev", optional = true, markers = "python_version >= \"3.11\" and extra == \"grpc\""}, {version = ">=1.33.2,<2.0dev", optional = true, markers = "python_version < \"3.11\" and extra == \"grpc\""}, + {version = ">=1.49.1,<2.0dev", optional = true, markers = "python_version >= \"3.11\" and extra == \"grpc\""}, ] grpcio-status = [ - {version = ">=1.49.1,<2.0.dev0", optional = true, markers = "python_version >= \"3.11\" and extra == \"grpc\""}, {version = ">=1.33.2,<2.0.dev0", optional = true, markers = "python_version < \"3.11\" and extra == \"grpc\""}, + {version = ">=1.49.1,<2.0.dev0", optional = true, markers = "python_version >= \"3.11\" and extra == \"grpc\""}, ] proto-plus = ">=1.22.3,<2.0.0dev" protobuf = ">=3.19.5,<3.20.0 || >3.20.0,<3.20.1 || >3.20.1,<4.21.0 || >4.21.0,<4.21.1 || >4.21.1,<4.21.2 || >4.21.2,<4.21.3 || >4.21.3,<4.21.4 || >4.21.4,<4.21.5 || >4.21.5,<6.0.0.dev0" @@ -4266,8 +4266,8 @@ files = [ [package.dependencies] numpy = [ - {version = ">=1.23.2", markers = "python_version == \"3.11\""}, {version = ">=1.22.4", markers = "python_version < \"3.11\""}, + {version = ">=1.23.2", markers = "python_version == \"3.11\""}, {version = ">=1.26.0", markers = "python_version >= \"3.12\""}, ] python-dateutil = ">=2.8.2" @@ -5212,6 +5212,25 @@ pytest = ">=7.0.0,<9" docs = ["sphinx (>=5.3)", "sphinx-rtd-theme (>=1.0)"] testing = ["coverage (>=6.2)", "hypothesis (>=5.7.1)"] +[[package]] +name = "pytest-subprocess" +version = "1.5.2" +description = "A plugin to fake subprocess for pytest" +optional = false +python-versions = ">=3.6" +files = [ + {file = "pytest_subprocess-1.5.2-py3-none-any.whl", hash = "sha256:23ac7732aa8bd45f1757265b1316eb72a7f55b41fb21e2ca22e149ba3629fa46"}, + {file = "pytest_subprocess-1.5.2.tar.gz", hash = "sha256:ad3ca8a35e798bf9c82d9f16d88700b30d98c5a28236117b86c5d6e581a8ed97"}, +] + +[package.dependencies] +pytest = ">=4.0.0" + +[package.extras] +dev = ["changelogd", "nox"] +docs = ["changelogd", "furo", "sphinx", "sphinx-autodoc-typehints", "sphinxcontrib-napoleon"] +test = ["Pygments (>=2.0)", "anyio", "coverage", "docutils (>=0.12)", "pytest (>=4.0)", "pytest-asyncio (>=0.15.1)", "pytest-rerunfailures", "pytest-timeout"] + [[package]] name = "pytest-vcr" version = "1.0.2" @@ -7331,4 +7350,4 @@ tools = ["crewai-tools"] [metadata] lock-version = "2.0" python-versions = ">=3.10,<=3.13" -content-hash = "b008b28add072e8f002aa90d230b20027f0ecffcd3c4b3fe4ec954f5ac7c46ca" +content-hash = "0b833460455da407e9b73fe545bb8dfccc114384d3ce1aa66a1d2fc83afb00f7" diff --git a/pyproject.toml b/pyproject.toml index dcf1a3770..cd9c65aa6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -57,6 +57,7 @@ pytest = "^8.0.0" pytest-vcr = "^1.0.2" python-dotenv = "1.0.0" pytest-asyncio = "^0.23.7" +pytest-subprocess = "^1.5.2" [tool.poetry.scripts] crewai = "crewai.cli.cli:crewai" diff --git a/src/crewai/cli/deploy/main.py b/src/crewai/cli/deploy/main.py index 0625ad82e..486959201 100644 --- a/src/crewai/cli/deploy/main.py +++ b/src/crewai/cli/deploy/main.py @@ -2,12 +2,9 @@ from typing import Any, Dict, List, Optional from rich.console import Console +from crewai.cli import git from crewai.cli.command import BaseCommand, PlusAPIMixin -from crewai.cli.utils import ( - fetch_and_json_env_file, - get_git_remote_url, - get_project_name, -) +from crewai.cli.utils import fetch_and_json_env_file, get_project_name console = Console() @@ -91,7 +88,11 @@ class DeployCommand(BaseCommand, PlusAPIMixin): ) console.print("Creating deployment...", style="bold blue") env_vars = fetch_and_json_env_file() - remote_repo_url = get_git_remote_url() + + try: + remote_repo_url = git.Repository().origin_url() + except ValueError: + remote_repo_url = None if remote_repo_url is None: console.print("No remote repository URL found.", style="bold red") diff --git a/src/crewai/cli/git.py b/src/crewai/cli/git.py new file mode 100644 index 000000000..94c3648b0 --- /dev/null +++ b/src/crewai/cli/git.py @@ -0,0 +1,80 @@ +import subprocess + + +class Repository: + def __init__(self, path="."): + self.path = path + + if not self.is_git_installed(): + raise ValueError("Git is not installed or not found in your PATH.") + + if not self.is_git_repo(): + raise ValueError(f"{self.path} is not a Git repository.") + + self.fetch() + + def is_git_installed(self) -> bool: + """Check if Git is installed and available in the system.""" + try: + subprocess.run( + ["git", "--version"], capture_output=True, check=True, text=True + ) + return True + except (subprocess.CalledProcessError, FileNotFoundError): + return False + + def fetch(self) -> None: + """Fetch latest updates from the remote.""" + subprocess.run(["git", "fetch"], cwd=self.path, check=True) + + def status(self) -> str: + """Get the git status in porcelain format.""" + return subprocess.check_output( + ["git", "status", "--branch", "--porcelain"], + cwd=self.path, + encoding="utf-8", + ).strip() + + def is_git_repo(self) -> bool: + """Check if the current directory is a git repository.""" + try: + subprocess.check_output( + ["git", "rev-parse", "--is-inside-work-tree"], + cwd=self.path, + encoding="utf-8", + ) + return True + except subprocess.CalledProcessError: + return False + + def has_uncommitted_changes(self) -> bool: + """Check if the repository has uncommitted changes.""" + return len(self.status().splitlines()) > 1 + + def is_ahead_or_behind(self) -> bool: + """Check if the repository is ahead or behind the remote.""" + for line in self.status().splitlines(): + if line.startswith("##") and ("ahead" in line or "behind" in line): + return True + return False + + def is_synced(self) -> bool: + """Return True if the Git repository is fully synced with the remote, False otherwise.""" + if self.has_uncommitted_changes() or self.is_ahead_or_behind(): + return False + else: + return True + + def origin_url(self) -> str | None: + """Get the Git repository's remote URL.""" + try: + result = subprocess.run( + ["git", "remote", "get-url", "origin"], + cwd=self.path, + capture_output=True, + text=True, + check=True, + ) + return result.stdout.strip() + except subprocess.CalledProcessError: + return None diff --git a/src/crewai/cli/tools/main.py b/src/crewai/cli/tools/main.py index 45c6e5d46..68c8f342c 100644 --- a/src/crewai/cli/tools/main.py +++ b/src/crewai/cli/tools/main.py @@ -6,6 +6,7 @@ import subprocess import tempfile from crewai.cli.command import BaseCommand, PlusAPIMixin +from crewai.cli import git from crewai.cli.utils import ( get_project_name, get_project_description, @@ -59,6 +60,17 @@ class ToolCommand(BaseCommand, PlusAPIMixin): os.chdir(old_directory) def publish(self, is_public: bool): + if not git.Repository().is_synced(): + console.print( + "[bold red]Failed to publish tool.[/bold red]\n" + "Local changes need to be resolved before publishing. Please do the following:\n" + "* [bold]Commit[/bold] your changes.\n" + "* [bold]Push[/bold] to sync with the remote.\n" + "* [bold]Pull[/bold] the latest changes from the remote.\n" + "\nOnce your repository is up-to-date, retry publishing the tool." + ) + raise SystemExit() + project_name = get_project_name(require=True) assert isinstance(project_name, str) diff --git a/src/crewai/cli/utils.py b/src/crewai/cli/utils.py index 6d25c4aeb..ba553f034 100644 --- a/src/crewai/cli/utils.py +++ b/src/crewai/cli/utils.py @@ -1,8 +1,6 @@ import os import shutil import click -import re -import subprocess import sys import importlib.metadata @@ -61,38 +59,6 @@ def parse_toml(content): return simple_toml_parser(content) -def get_git_remote_url() -> str | None: - """Get the Git repository's remote URL.""" - try: - # Run the git remote -v command - result = subprocess.run( - ["git", "remote", "-v"], capture_output=True, text=True, check=True - ) - - # Get the output - output = result.stdout - - # Parse the output to find the origin URL - matches = re.findall(r"origin\s+(.*?)\s+\(fetch\)", output) - - if matches: - return matches[0] # Return the first match (origin URL) - else: - console.print("No origin remote found.", style="bold red") - - except subprocess.CalledProcessError as e: - console.print( - f"Error running trying to fetch the Git Repository: {e}", style="bold red" - ) - except FileNotFoundError: - console.print( - "Git command not found. Make sure Git is installed and in your PATH.", - style="bold red", - ) - - return None - - def get_project_name( pyproject_path: str = "pyproject.toml", require: bool = False ) -> str | None: diff --git a/tests/cli/deploy/test_deploy_main.py b/tests/cli/deploy/test_deploy_main.py index c32d254cb..b30883be3 100644 --- a/tests/cli/deploy/test_deploy_main.py +++ b/tests/cli/deploy/test_deploy_main.py @@ -143,11 +143,11 @@ class TestDeployCommand(unittest.TestCase): mock_display.assert_called_once_with({"uuid": "test-uuid"}) @patch("crewai.cli.deploy.main.fetch_and_json_env_file") - @patch("crewai.cli.deploy.main.get_git_remote_url") + @patch("crewai.cli.deploy.main.git.Repository.origin_url") @patch("builtins.input") - def test_create_crew(self, mock_input, mock_get_git_remote_url, mock_fetch_env): + def test_create_crew(self, mock_input, mock_git_origin_url, mock_fetch_env): mock_fetch_env.return_value = {"ENV_VAR": "value"} - mock_get_git_remote_url.return_value = "https://github.com/test/repo.git" + mock_git_origin_url.return_value = "https://github.com/test/repo.git" mock_input.return_value = "" mock_response = MagicMock() diff --git a/tests/cli/test_git.py b/tests/cli/test_git.py new file mode 100644 index 000000000..1fb1296cd --- /dev/null +++ b/tests/cli/test_git.py @@ -0,0 +1,101 @@ +from crewai.cli.git import Repository +import pytest + + +@pytest.fixture() +def repository(fp): + fp.register(["git", "--version"], stdout="git version 2.30.0\n") + fp.register(["git", "rev-parse", "--is-inside-work-tree"], stdout="true\n") + fp.register(["git", "fetch"], stdout="") + return Repository(path=".") + + +def test_init_with_invalid_git_repo(fp): + fp.register(["git", "--version"], stdout="git version 2.30.0\n") + fp.register( + ["git", "rev-parse", "--is-inside-work-tree"], + returncode=1, + stderr="fatal: not a git repository\n", + ) + + with pytest.raises(ValueError): + Repository(path="invalid/path") + + +def test_is_git_not_installed(fp): + fp.register(["git", "--version"], returncode=1) + + with pytest.raises( + ValueError, match="Git is not installed or not found in your PATH." + ): + Repository(path=".") + + +def test_status(fp, repository): + fp.register( + ["git", "status", "--branch", "--porcelain"], + stdout="## main...origin/main [ahead 1]\n", + ) + assert repository.status() == "## main...origin/main [ahead 1]" + + +def test_has_uncommitted_changes(fp, repository): + fp.register( + ["git", "status", "--branch", "--porcelain"], + stdout="## main...origin/main\n M somefile.txt\n", + ) + assert repository.has_uncommitted_changes() is True + + +def test_is_ahead_or_behind(fp, repository): + fp.register( + ["git", "status", "--branch", "--porcelain"], + stdout="## main...origin/main [ahead 1]\n", + ) + assert repository.is_ahead_or_behind() is True + + +def test_is_synced_when_synced(fp, repository): + fp.register( + ["git", "status", "--branch", "--porcelain"], stdout="## main...origin/main\n" + ) + fp.register( + ["git", "status", "--branch", "--porcelain"], stdout="## main...origin/main\n" + ) + assert repository.is_synced() is True + + +def test_is_synced_with_uncommitted_changes(fp, repository): + fp.register( + ["git", "status", "--branch", "--porcelain"], + stdout="## main...origin/main\n M somefile.txt\n", + ) + assert repository.is_synced() is False + + +def test_is_synced_when_ahead_or_behind(fp, repository): + fp.register( + ["git", "status", "--branch", "--porcelain"], + stdout="## main...origin/main [ahead 1]\n", + ) + fp.register( + ["git", "status", "--branch", "--porcelain"], + stdout="## main...origin/main [ahead 1]\n", + ) + assert repository.is_synced() is False + + +def test_is_synced_with_uncommitted_changes_and_ahead(fp, repository): + fp.register( + ["git", "status", "--branch", "--porcelain"], + stdout="## main...origin/main [ahead 1]\n M somefile.txt\n", + ) + assert repository.is_synced() is False + + +def test_origin_url(fp, repository): + fp.register( + ["git", "remote", "get-url", "origin"], + stdout="https://github.com/user/repo.git\n", + ) + assert repository.origin_url() == "https://github.com/user/repo.git" diff --git a/tests/cli/tools/test_main.py b/tests/cli/tools/test_main.py index ec1d71668..9f269091b 100644 --- a/tests/cli/tools/test_main.py +++ b/tests/cli/tools/test_main.py @@ -129,8 +129,10 @@ class TestToolCommand(unittest.TestCase): read_data=b"sample tarball content", ) @patch("crewai.cli.plus_api.PlusAPI.publish_tool") + @patch("crewai.cli.tools.main.git.Repository.is_synced", return_value=True) def test_publish_success( self, + mock_is_synced, mock_publish, mock_open, mock_listdir, @@ -147,16 +149,16 @@ class TestToolCommand(unittest.TestCase): tool_command = ToolCommand() tool_command.publish(is_public=True) - mock_get_project_name.assert_called_once_with(require=True) - mock_get_project_version.assert_called_once_with(require=True) - mock_get_project_description.assert_called_once_with(require=False) - mock_subprocess_run.assert_called_once_with( + mock_get_project_name.assert_called_with(require=True) + mock_get_project_version.assert_called_with(require=True) + mock_get_project_description.assert_called_with(require=False) + mock_subprocess_run.assert_called_with( ["poetry", "build", "-f", "sdist", "--output", unittest.mock.ANY], check=True, capture_output=False, ) - mock_open.assert_called_once_with(unittest.mock.ANY, "rb") - mock_publish.assert_called_once_with( + mock_open.assert_called_with(unittest.mock.ANY, "rb") + mock_publish.assert_called_with( handle="sample-tool", is_public=True, version="1.0.0",