From fd638e3201b314e5914bf1ecd821a73127f91022 Mon Sep 17 00:00:00 2001 From: Yannik Tausch Date: Fri, 23 Aug 2024 10:44:14 +0200 Subject: [PATCH 1/3] use GitCli for adding and commiting migrations --- conda_forge_tick/auto_tick.py | 86 +++++++++++------- conda_forge_tick/git_utils.py | 37 ++++++++ tests/test_auto_tick.py | 159 +++++++++++++++++++++++++++++++++- tests/test_git_utils.py | 116 +++++++++++++++++++++++++ 4 files changed, 367 insertions(+), 31 deletions(-) diff --git a/conda_forge_tick/auto_tick.py b/conda_forge_tick/auto_tick.py index 566d1fd45..8ef734601 100644 --- a/conda_forge_tick/auto_tick.py +++ b/conda_forge_tick/auto_tick.py @@ -7,7 +7,6 @@ import time import traceback import typing -from subprocess import CalledProcessError from textwrap import dedent from typing import Literal, MutableMapping, cast from urllib.error import URLError @@ -31,6 +30,8 @@ from conda_forge_tick.feedstock_parser import BOOTSTRAP_MAPPINGS from conda_forge_tick.git_utils import ( DryRunBackend, + GitCli, + GitCliError, GitPlatformBackend, RepositoryNotFoundError, comment_on_pr, @@ -189,6 +190,39 @@ def _prepare_feedstock_repository( return True +def _commit_migration( + cli: GitCli, + context: ClonedFeedstockContext, + commit_message: str, + allow_empty_commits: bool = False, + raise_commit_errors: bool = True, +) -> None: + """ + Commit a migration that has been run in the local clone of a feedstock repository. + If an error occurs during the commit, it is logged. + :param cli: The GitCli instance to use. + :param context: The FeedstockContext instance. + :param commit_message: The commit message to use. + :param allow_empty_commits: Whether the migrator allows empty commits. + :param raise_commit_errors: Whether to raise an exception if an error occurs during the commit. + :raises GitCliError: If an error occurs during the commit and raise_commit_errors is True. + """ + cli.add( + context.local_clone_dir, + all_=True, + ) + + try: + cli.commit( + context.local_clone_dir, commit_message, allow_empty=allow_empty_commits + ) + except GitCliError as e: + logger.info("could not commit to feedstock - likely no changes", exc_info=e) + + if raise_commit_errors: + raise + + def run_with_tmpdir( context: FeedstockContext, migrator: Migrator, @@ -274,19 +308,10 @@ def run( # something went wrong during forking or cloning return False, False - # need to use an absolute path here - feedstock_dir = str(context.local_clone_dir.resolve()) - - # This is needed because we want to migrate to the new backend step-by-step - repo: github3.repos.Repository | None = github3_client().repository( - context.git_repo_owner, context.git_repo_name - ) - - assert repo is not None - + # feedstock_dir must be absolute migration_run_data = run_migration( migrator=migrator, - feedstock_dir=feedstock_dir, + feedstock_dir=str(context.local_clone_dir.resolve()), feedstock_name=context.feedstock_name, node_attrs=context.attrs, default_branch=context.default_branch, @@ -301,27 +326,28 @@ def run( ) return False, False + # We raise an exception if we don't plan to rerender and wanted an empty commit. + # This prevents PRs that don't actually get made from getting marked as done. + _commit_migration( + cli=git_backend.cli, + context=context, + commit_message=migration_run_data["commit_message"], + allow_empty_commits=migrator.allow_empty_commits, + raise_commit_errors=migrator.allow_empty_commits and not rerender, + ) + + # This is needed because we want to migrate to the new backend step-by-step + repo: github3.repos.Repository | None = github3_client().repository( + context.git_repo_owner, context.git_repo_name + ) + + assert repo is not None + + feedstock_dir = str(context.local_clone_dir.resolve()) + # rerender, maybe diffed_files: typing.List[str] = [] with pushd(feedstock_dir): - msg = migration_run_data["commit_message"] - try: - eval_cmd(["git", "add", "--all", "."]) - if migrator.allow_empty_commits: - eval_cmd(["git", "commit", "--allow-empty", "-am", msg]) - else: - eval_cmd(["git", "commit", "-am", msg]) - except CalledProcessError as e: - logger.info( - "could not commit to feedstock - " - "likely no changes - error is '%s'" % (repr(e)), - ) - # we bail here if we do not plan to rerender and we wanted an empty - # commit - # this prevents PRs that don't actually get made from getting marked as done - if migrator.allow_empty_commits and not rerender: - raise e - if rerender: head_ref = eval_cmd(["git", "rev-parse", "HEAD"]).strip() logger.info("Rerendering the feedstock") diff --git a/conda_forge_tick/git_utils.py b/conda_forge_tick/git_utils.py index 44518bef4..01c1f9ec5 100644 --- a/conda_forge_tick/git_utils.py +++ b/conda_forge_tick/git_utils.py @@ -166,6 +166,43 @@ def _run_git_command( except subprocess.CalledProcessError as e: raise GitCliError("Error running git command.") from e + @lock_git_operation() + def add(self, git_dir: Path, *pathspec: Path, all_: bool = False): + """ + Add files to the git index with `git add`. + :param git_dir: The directory of the git repository. + :param pathspec: The files to add. + :param all_: If True, not only add the files in pathspec, but also where the index already has an entry. + If all_ is set with empty pathspec, all files in the entire working tree are updated. + :raises ValueError: If pathspec is empty and all_ is False. + :raises GitCliError: If the git command fails. + """ + if not pathspec and not all_: + raise ValueError("Either pathspec or all_ must be set.") + + all_arg = ["--all"] if all_ else [] + + self._run_git_command(["add", *all_arg, *pathspec], git_dir) + + @lock_git_operation() + def commit( + self, git_dir: Path, message: str, all_: bool = False, allow_empty: bool = False + ): + """ + Commit changes to the git repository with `git commit`. + :param git_dir: The directory of the git repository. + :param message: The commit message. + :param allow_empty: If True, allow an empty commit. + :param all_: Automatically stage files that have been modified and deleted, but new files are not affected. + :raises GitCliError: If the git command fails. + """ + all_arg = ["-a"] if all_ else [] + allow_empty_arg = ["--allow-empty"] if allow_empty else [] + + self._run_git_command( + ["commit", *all_arg, *allow_empty_arg, "-m", message], git_dir + ) + @lock_git_operation() def reset_hard(self, git_dir: Path, to_treeish: str = "HEAD"): """ diff --git a/tests/test_auto_tick.py b/tests/test_auto_tick.py index f6dac25d8..fdae0da6a 100644 --- a/tests/test_auto_tick.py +++ b/tests/test_auto_tick.py @@ -8,10 +8,16 @@ import pytest from conftest import FakeLazyJson -from conda_forge_tick.auto_tick import _prepare_feedstock_repository, run_with_tmpdir +from conda_forge_tick.auto_tick import ( + _commit_migration, + _prepare_feedstock_repository, + run_with_tmpdir, +) from conda_forge_tick.contexts import ClonedFeedstockContext, FeedstockContext from conda_forge_tick.git_utils import ( DryRunBackend, + GitCli, + GitCliError, GitPlatformBackend, RepositoryNotFoundError, ) @@ -143,6 +149,157 @@ def test_prepare_feedstock_repository_complete_fail(): ) +@pytest.mark.parametrize("raise_commit_errors", [True, False]) +@pytest.mark.parametrize("allow_empty_commits", [True, False]) +def test_commit_migration_nonempty( + raise_commit_errors: bool, allow_empty_commits: bool +): + backend = DryRunBackend() + + context = FeedstockContext( + feedstock_name="pytest", + attrs=demo_attrs, + ) + + with context.reserve_clone_directory() as cloned_context: + assert _prepare_feedstock_repository( + backend, cloned_context, "new_branch", "main" + ) + + # now we do our "migration" + cloned_context.local_clone_dir.joinpath("conda-forge.yml").unlink() + with cloned_context.local_clone_dir.joinpath("new_file.txt").open("w") as f: + f.write("Hello World!") + + _commit_migration( + backend.cli, + cloned_context, + "COMMIT_MESSAGE_1337", + allow_empty_commits, + raise_commit_errors, + ) + + # commit should be there + assert ( + "COMMIT_MESSAGE_1337" + in backend.cli._run_git_command( + ["log", "-1", "--pretty=%B"], + cloned_context.local_clone_dir, + capture_text=True, + ).stdout + ) + + +@pytest.mark.parametrize("raise_commit_errors", [True, False]) +@pytest.mark.parametrize("allow_empty_commits", [True, False]) +def test_commit_migration_empty(raise_commit_errors: bool, allow_empty_commits: bool): + backend = DryRunBackend() + + context = FeedstockContext( + feedstock_name="pytest", + attrs=demo_attrs, + ) + + with context.reserve_clone_directory() as cloned_context: + assert _prepare_feedstock_repository( + backend, cloned_context, "new_branch", "main" + ) + + if raise_commit_errors and not allow_empty_commits: + with pytest.raises(GitCliError): + _commit_migration( + backend.cli, + cloned_context, + "COMMIT_MESSAGE", + allow_empty_commits, + raise_commit_errors, + ) + return + else: + # everything should work normally + _commit_migration( + backend.cli, + cloned_context, + "COMMIT_MESSAGE_1337", + allow_empty_commits, + raise_commit_errors, + ) + + if not allow_empty_commits: + return + + # commit should be there + assert ( + "COMMIT_MESSAGE_1337" + in backend.cli._run_git_command( + ["log", "-1", "--pretty=%B"], + cloned_context.local_clone_dir, + capture_text=True, + ).stdout + ) + + +@pytest.mark.parametrize("raise_commit_errors", [True, False]) +@pytest.mark.parametrize("allow_empty_commits", [True, False]) +def test_commit_migration_mock_no_error( + allow_empty_commits: bool, raise_commit_errors: bool +): + cli = create_autospec(GitCli) + + clone_dir = Path("LOCAL_CLONE_DIR") + + context = ClonedFeedstockContext( + feedstock_name="pytest", attrs=demo_attrs, local_clone_dir=clone_dir + ) + + _commit_migration( + cli, context, "COMMIT MESSAGE 1337", allow_empty_commits, raise_commit_errors + ) + + cli.commit.assert_called_once_with( + clone_dir, "COMMIT MESSAGE 1337", allow_empty=allow_empty_commits + ) + + +@pytest.mark.parametrize("raise_commit_errors", [True, False]) +@pytest.mark.parametrize("allow_empty_commits", [True, False]) +def test_commit_migration_mock_error( + allow_empty_commits: bool, raise_commit_errors: bool +): + cli = create_autospec(GitCli) + + clone_dir = Path("LOCAL_CLONE_DIR") + + cli.commit.side_effect = GitCliError("Error") + + context = ClonedFeedstockContext( + feedstock_name="pytest", attrs=demo_attrs, local_clone_dir=clone_dir + ) + + if raise_commit_errors: + with pytest.raises(GitCliError): + _commit_migration( + cli, + context, + "COMMIT MESSAGE 1337", + allow_empty_commits, + raise_commit_errors, + ) + else: + _commit_migration( + cli, + context, + "COMMIT MESSAGE 1337", + allow_empty_commits, + raise_commit_errors, + ) + + cli.add.assert_called_once_with(clone_dir, all_=True) + cli.commit.assert_called_once_with( + clone_dir, "COMMIT MESSAGE 1337", allow_empty=allow_empty_commits + ) + + @pytest.mark.parametrize("dry_run", [True, False]) @pytest.mark.parametrize("base_branch", ["main", "master"]) @pytest.mark.parametrize("rerender", [True, False]) diff --git a/tests/test_git_utils.py b/tests/test_git_utils.py index 754624cdf..767ebde22 100644 --- a/tests/test_git_utils.py +++ b/tests/test_git_utils.py @@ -97,6 +97,122 @@ def init_temp_git_repo(git_dir: Path): ) +@pytest.mark.parametrize( + "n_paths,all_", [(0, True), (1, False), (1, True), (2, False), (2, True)] +) +@mock.patch("conda_forge_tick.git_utils.GitCli._run_git_command") +def test_git_cli_add_success_mock( + run_git_command_mock: MagicMock, n_paths: int, all_: bool +): + cli = GitCli() + + git_dir = Path("TEST_DIR") + paths = [Path(f"test{i}.txt") for i in range(n_paths)] + + cli.add(git_dir, *paths, all_=all_) + + expected_all_arg = ["--all"] if all_ else [] + + run_git_command_mock.assert_called_once_with( + ["add", *expected_all_arg, *paths], git_dir + ) + + +@mock.patch("conda_forge_tick.git_utils.GitCli._run_git_command") +def test_git_cli_add_no_arguments_error(run_git_command_mock: MagicMock): + cli = GitCli() + + git_dir = Path("TEST_DIR") + + with pytest.raises(ValueError, match="Either pathspec or all_ must be set"): + cli.add(git_dir) + + run_git_command_mock.assert_not_called() + + +@pytest.mark.parametrize( + "n_paths,all_", [(0, True), (1, False), (1, True), (2, False), (2, True)] +) +def test_git_cli_add_success(n_paths: int, all_: bool): + with tempfile.TemporaryDirectory() as tmp_dir: + git_dir = Path(tmp_dir) + init_temp_git_repo(git_dir) + + pathspec = [git_dir / f"test{i}.txt" for i in range(n_paths)] + + for path in pathspec + [git_dir / "all_tracker.txt"]: + path.touch() + + cli = GitCli() + cli.add(git_dir, *pathspec, all_=all_) + + tracked_files = cli._run_git_command( + ["ls-files", "-s"], git_dir, capture_text=True + ).stdout + + for path in pathspec: + assert path.name in tracked_files + + if all_ and n_paths == 0: + # note that n_paths has to be zero to add unknown files to the working tree + assert "all_tracker.txt" in tracked_files + else: + assert "all_tracker.txt" not in tracked_files + + +@pytest.mark.parametrize("allow_empty", [True, False]) +@pytest.mark.parametrize("all_", [True, False]) +@mock.patch("conda_forge_tick.git_utils.GitCli._run_git_command") +def test_git_cli_commit_success_mock( + run_git_command_mock: MagicMock, all_: bool, allow_empty: bool +): + git_dir = Path("GIT_DIR") + message = "COMMIT_MESSAGE" + + cli = GitCli() + cli.commit(git_dir, message, all_, allow_empty) + + expected_all_arg = ["-a"] if all_ else [] + expected_allow_empty_arg = ["--allow-empty"] if allow_empty else [] + + run_git_command_mock.assert_called_once_with( + ["commit", *expected_all_arg, *expected_allow_empty_arg, "-m", message], git_dir + ) + + +@pytest.mark.parametrize("allow_empty", [True, False]) +@pytest.mark.parametrize("empty", [True, False]) +@pytest.mark.parametrize("all_", [True, False]) +def test_git_cli_commit(all_: bool, empty: bool, allow_empty: bool): + with tempfile.TemporaryDirectory() as tmp_dir: + git_dir = Path(tmp_dir) + init_temp_git_repo(git_dir) + + cli = GitCli() + + test_file = git_dir.joinpath("test.txt") + with test_file.open("w") as f: + f.write("Hello, World!") + cli.add(git_dir, git_dir / "test.txt") + cli.commit(git_dir, "Add Test") + + if not empty: + test_file.unlink() + if not all_: + cli.add(git_dir, git_dir / "test.txt") + + if empty and not allow_empty: + with pytest.raises(GitCliError): + cli.commit(git_dir, "Add Test", all_, allow_empty) + return + + cli.commit(git_dir, "Add Test", all_, allow_empty) + + git_log = cli._run_git_command(["log"], git_dir, capture_text=True).stdout + + assert "Add Test" in git_log + + def test_git_cli_reset_hard_already_reset(): cli = GitCli() with tempfile.TemporaryDirectory() as tmpdir: From 2e86fdb7a322133e315c43a3fb16349edf6e0995 Mon Sep 17 00:00:00 2001 From: Yannik Tausch Date: Fri, 6 Sep 2024 16:44:57 +0200 Subject: [PATCH 2/3] fix: add git stuff to reusable tests Co-authored-by: Matthew R Becker --- .github/workflows/tests-reusable.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/tests-reusable.yml b/.github/workflows/tests-reusable.yml index 65cb097f8..17020c968 100644 --- a/.github/workflows/tests-reusable.yml +++ b/.github/workflows/tests-reusable.yml @@ -72,6 +72,12 @@ jobs: run: | pip install --no-deps --no-build-isolation -e . + - name: Set up git + run: | + git config --global user.name regro-cf-autotick-bot + git config --global user.email 36490558+regro-cf-autotick-bot@users.noreply.github.com + git config --global pull.rebase false + - name: test versions run: | cd .. From 6092eface514baff85a6ba6f2028f2f8407b5cc2 Mon Sep 17 00:00:00 2001 From: "Matthew R. Becker" Date: Sat, 7 Sep 2024 06:40:34 -0500 Subject: [PATCH 3/3] Update conda_forge_tick/auto_tick.py --- conda_forge_tick/auto_tick.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conda_forge_tick/auto_tick.py b/conda_forge_tick/auto_tick.py index 8ef734601..151a816f2 100644 --- a/conda_forge_tick/auto_tick.py +++ b/conda_forge_tick/auto_tick.py @@ -308,7 +308,7 @@ def run( # something went wrong during forking or cloning return False, False - # feedstock_dir must be absolute + # feedstock_dir must be an absolute path migration_run_data = run_migration( migrator=migrator, feedstock_dir=str(context.local_clone_dir.resolve()),