From 88b3d9947ca0a6f2b6d73ce5f04beca140699644 Mon Sep 17 00:00:00 2001 From: ian-bowden-slalom <128502385+ian-bowden-slalom@users.noreply.github.com> Date: Wed, 26 Jun 2024 10:42:43 -0400 Subject: [PATCH] chore: updatehooks skips updated repos that aren't out of date by default (#575) This PR adds a new argument to the updatehooks function named force_update which is set to False by default. if force_update is false, then we won't download/install the repo unless it is out of date. If force_update is True, then we'd download and install the latest repo version irrespective of the currently installed version. There is currently no code (outside of unit tests) that's passing True for the force_update argument. ## Changes * ## Testing * ## Clean Code Checklist - [ ] Meets acceptance criteria for issue - [ ] New logic is covered with automated tests - [ ] Appropriate exception handling added - [ ] Thoughtful logging included - [ ] Documentation is updated - [ ] Follow-up work is documented in TODOs - [ ] TODOs have a ticket associated with them - [ ] No commented-out code included --------- Co-authored-by: Ian Bowden --- .../modules/core/core_services/updater.py | 4 +- .../modules/shared/abstractions/pre_commit.py | 45 +++- .../shared/abstractions/test_pre_commit.py | 205 ++++++++++++++++-- 3 files changed, 229 insertions(+), 25 deletions(-) diff --git a/secureli/modules/core/core_services/updater.py b/secureli/modules/core/core_services/updater.py index 120646a9..e54929a0 100644 --- a/secureli/modules/core/core_services/updater.py +++ b/secureli/modules/core/core_services/updater.py @@ -25,6 +25,7 @@ def update_hooks( bleeding_edge: bool = False, freeze: bool = False, repos: Optional[list] = None, + force_update: Optional[bool] = False, ): """ Updates the precommit hooks but executing precommit's autoupdate command. Additional info at @@ -34,10 +35,11 @@ def update_hooks( the latest tagged version (which is the default behavior) :param freeze: Set to True to store "frozen" hashes in rev instead of tag names. :param repos: Dectionary of repos to update. This is used to target specific repos instead of all repos. + :param force_update: set to True to download updates for hooks whose versions aren't out of date. False means only out-of-date repos are updated :return: ExecuteResult, indicating success or failure. """ update_result = self.pre_commit.autoupdate_hooks( - folder_path, bleeding_edge, freeze, repos + folder_path, bleeding_edge, freeze, repos, force_update ) output = update_result.output diff --git a/secureli/modules/shared/abstractions/pre_commit.py b/secureli/modules/shared/abstractions/pre_commit.py index c91fc60e..4faf3f60 100644 --- a/secureli/modules/shared/abstractions/pre_commit.py +++ b/secureli/modules/shared/abstractions/pre_commit.py @@ -1,6 +1,8 @@ import datetime from pathlib import Path import shutil +from urllib.parse import urlparse +from git import Repo # Note that this import is pulling from the pre-commit tool's internals. # A cleaner approach would be to update pre-commit @@ -189,8 +191,14 @@ def check_for_hook_updates( "repo": repo_config.url } # PreCommitSettings uses "url" instead of "repo", so we need to copy that value over old_rev_info = HookRepoRevInfo.from_config(repo_config_dict) + + # don't try and update the local repo + if old_rev_info.repo == "local": + continue + # if the revision currently specified in .pre-commit-config.yaml looks like a full git SHA # (40-character hex string), then set freeze to True + freeze = ( bool(git_commit_sha_pattern.fullmatch(repo_config.rev)) if freeze is None @@ -208,6 +216,7 @@ def autoupdate_hooks( bleeding_edge: bool = False, freeze: bool = False, repos: Optional[list] = None, + force_update: Optional[bool] = False, ) -> ExecuteResult: """ Updates the precommit hooks by executing precommit's autoupdate command. Additional info at @@ -217,8 +226,17 @@ def autoupdate_hooks( the latest tagged version (which is the default behavior) :param freeze: Set to True to store "frozen" hashes in rev instead of tag names. :param repos: List of repos (url as a string) to update. This is used to target specific repos instead of all repos. + :param force_update: set to True to download updates for hooks whose versions aren't out of date. False means only out-of-date repos are updated :return: ExecuteResult, indicating success or failure. """ + if not force_update: + repos = self._get_outdated_repos(folder_path, bleeding_edge, freeze, repos) + + # if there's no outdated repos and we're not forcing updates then there's nothing more to do + if not repos and not force_update: + output = "No changes necessary.\n" + return ExecuteResult(successful=True, output=output) + subprocess_args = [ "pre-commit", "autoupdate", @@ -240,7 +258,9 @@ def autoupdate_hooks( for repo in repos: if isinstance(repo, str): - arg = "--repo {}".format(repo) + arg = "--repo" + repo_args.append(arg) + arg = format(repo) repo_args.append(arg) else: output = "Unable to update repo, string validation failed. Repo parameter should be a dictionary of strings." @@ -390,3 +410,26 @@ def pre_commit_config_exists(self, folder_path: Path) -> bool: """ path_to_config = folder_path / ".pre-commit-config.yaml" return path_to_config.exists() + + def _get_outdated_repos( + self, + folder_path: Path, + bleeding_edge: bool = False, + freeze: bool = False, + repos: Optional[list] = None, + ) -> list: + # if no repos are specified then use the pre commit config to get a list of all possible repos to update + if not repos: + precommit_config = self.get_pre_commit_config(folder_path) + outdated_repos = self.check_for_hook_updates( + precommit_config, not bleeding_edge, freeze + ) + repos = [key for key in outdated_repos.keys()] + # Only check for updates for the specified repos + else: + outdated_repos = self.check_for_hook_updates( + PreCommitSettings(repos=repos), not bleeding_edge, freeze + ) + repos = [key for key in outdated_repos.keys()] + + return repos diff --git a/tests/modules/shared/abstractions/test_pre_commit.py b/tests/modules/shared/abstractions/test_pre_commit.py index 4cf08ac2..6467bd2f 100644 --- a/tests/modules/shared/abstractions/test_pre_commit.py +++ b/tests/modules/shared/abstractions/test_pre_commit.py @@ -180,9 +180,31 @@ def test_that_pre_commit_autoupdate_hooks_executes_successfully( pre_commit: PreCommitAbstractionModels.PreCommitAbstraction, mock_subprocess: MagicMock, ): - with (um.patch.object(Path, "exists") as mock_exists,): - mock_exists.return_value = True + with ( + um.patch.object(Path, "exists") as mock_exists, + um.patch("builtins.open", um.mock_open()) as mock_open, + um.patch( + "secureli.modules.shared.abstractions.pre_commit.HookRepoRevInfo.from_config" + ) as mock_hook_repo_rev_info, + ): mock_subprocess.run.return_value = CompletedProcess(args=[], returncode=0) + mock_exists.return_value = True + mock_open.return_value.read.return_value = ( + "repos:\n" + " - repo: my-repo\n" + " rev: tag1\n" + " hooks:\n" + " - id: detect-secrets\n" + " args: ['--foo', '--bar']\n" + ) + pre_commit_config_repo = RepositoryModels.PreCommitRepo( + repo="http://example-repo.com/", + rev="tag1", + hooks=[RepositoryModels.PreCommitHook(id="hook-id")], + ) + rev_info_mock = MagicMock(rev=pre_commit_config_repo.rev) + mock_hook_repo_rev_info.return_value = rev_info_mock + rev_info_mock.update.return_value = rev_info_mock execute_result = pre_commit.autoupdate_hooks(test_folder_path) assert execute_result.successful @@ -193,9 +215,12 @@ def test_that_pre_commit_autoupdate_hooks_properly_handles_failed_executions( mock_subprocess: MagicMock, ): with (um.patch.object(Path, "exists") as mock_exists,): - mock_exists.return_value = True mock_subprocess.run.return_value = CompletedProcess(args=[], returncode=1) - execute_result = pre_commit.autoupdate_hooks(test_folder_path) + mock_exists.return_value = True + + execute_result = pre_commit.autoupdate_hooks( + test_folder_path, force_update=True + ) assert not execute_result.successful @@ -205,10 +230,11 @@ def test_that_pre_commit_autoupdate_hooks_executes_successfully_with_bleeding_ed mock_subprocess: MagicMock, ): with (um.patch.object(Path, "exists") as mock_exists,): - mock_exists.return_value = True mock_subprocess.run.return_value = CompletedProcess(args=[], returncode=0) + mock_exists.return_value = True + execute_result = pre_commit.autoupdate_hooks( - test_folder_path, bleeding_edge=True + test_folder_path, bleeding_edge=True, repos=["test"], force_update=True ) assert execute_result.successful @@ -222,7 +248,9 @@ def test_that_pre_commit_autoupdate_hooks_executes_successfully_with_freeze( with (um.patch.object(Path, "exists") as mock_exists,): mock_exists.return_value = True mock_subprocess.run.return_value = CompletedProcess(args=[], returncode=0) - execute_result = pre_commit.autoupdate_hooks(test_folder_path, freeze=True) + execute_result = pre_commit.autoupdate_hooks( + test_folder_path, freeze=True, force_update=True + ) assert execute_result.successful assert "--freeze" in mock_subprocess.run.call_args_list[0].args[0] @@ -236,10 +264,13 @@ def test_that_pre_commit_autoupdate_hooks_executes_successfully_with_repos( with (um.patch.object(Path, "exists") as mock_exists,): mock_exists.return_value = True mock_subprocess.run.return_value = CompletedProcess(args=[], returncode=0) - execute_result = pre_commit.autoupdate_hooks(test_folder_path, repos=test_repos) + execute_result = pre_commit.autoupdate_hooks( + test_folder_path, repos=test_repos, force_update=True + ) assert execute_result.successful - assert "--repo some-repo-url" in mock_subprocess.run.call_args_list[0].args[0] + assert "--repo" in mock_subprocess.run.call_args_list[0].args[0] + assert "some-repo-url" in mock_subprocess.run.call_args_list[0].args[0] def test_that_pre_commit_autoupdate_hooks_executes_successfully_with_multiple_repos( @@ -250,14 +281,14 @@ def test_that_pre_commit_autoupdate_hooks_executes_successfully_with_multiple_re mock_subprocess.run.return_value = CompletedProcess(args=[], returncode=0) with (um.patch.object(Path, "exists") as mock_exists,): mock_exists.return_value = True - execute_result = pre_commit.autoupdate_hooks(test_folder_path, repos=test_repos) + execute_result = pre_commit.autoupdate_hooks( + test_folder_path, repos=test_repos, force_update=True + ) assert execute_result.successful - assert "--repo some-repo-url" in mock_subprocess.run.call_args_list[0].args[0] - assert ( - "--repo some-other-repo-url" - in mock_subprocess.run.call_args_list[0].args[0] - ) + assert mock_subprocess.run.call_args_list[0].args[0].count("--repo") == 2 + assert "some-repo-url" in mock_subprocess.run.call_args_list[0].args[0] + assert "some-other-repo-url" in mock_subprocess.run.call_args_list[0].args[0] def test_that_pre_commit_autoupdate_hooks_fails_with_repos_containing_non_strings( @@ -268,7 +299,9 @@ def test_that_pre_commit_autoupdate_hooks_fails_with_repos_containing_non_string mock_subprocess.run.return_value = CompletedProcess(args=[], returncode=0) with (um.patch.object(Path, "exists") as mock_exists,): mock_exists.return_value = True - execute_result = pre_commit.autoupdate_hooks(test_folder_path, repos=test_repos) + execute_result = pre_commit.autoupdate_hooks( + test_folder_path, repos=test_repos, force_update=True + ) assert not execute_result.successful @@ -281,7 +314,7 @@ def test_that_pre_commit_autoupdate_hooks_ignores_repos_when_repos_is_a_dict( mock_subprocess.run.return_value = CompletedProcess(args=[], returncode=0) with (um.patch.object(Path, "exists") as mock_exists,): mock_exists.return_value = True - execute_result = pre_commit.autoupdate_hooks(test_folder_path, repos=test_repos) # type: ignore + execute_result = pre_commit.autoupdate_hooks(test_folder_path, repos=test_repos, force_update=True) # type: ignore assert execute_result.successful assert "--repo {}" not in mock_subprocess.run.call_args_list[0].args[0] @@ -295,10 +328,84 @@ def test_that_pre_commit_autoupdate_hooks_converts_repos_when_repos_is_a_string( mock_subprocess.run.return_value = CompletedProcess(args=[], returncode=0) with (um.patch.object(Path, "exists") as mock_exists,): mock_exists.return_value = True - execute_result = pre_commit.autoupdate_hooks(test_folder_path, repos=test_repos) # type: ignore + execute_result = pre_commit.autoupdate_hooks(test_folder_path, repos=test_repos, force_update=True) # type: ignore + + assert execute_result.successful + assert "--repo" in mock_subprocess.run.call_args_list[0].args[0] + assert "string" in mock_subprocess.run.call_args_list[0].args[0] + + +def test_that_pre_commit_autoupdate_hooks_executes_successfully_when_not_forcing_updates( + pre_commit: PreCommitAbstractionModels.PreCommitAbstraction, + mock_subprocess: MagicMock, +): + with ( + um.patch.object(Path, "exists") as mock_exists, + um.patch("builtins.open", um.mock_open()) as mock_open, + um.patch( + "secureli.modules.shared.abstractions.pre_commit.HookRepoRevInfo.from_config" + ) as mock_hook_repo_rev_info, + ): + mock_subprocess.run.return_value = CompletedProcess(args=[], returncode=0) + mock_exists.return_value = True + mock_open.return_value.read.return_value = ( + "repos:\n" + " - repo: my-repo\n" + " rev: tag1\n" + " hooks:\n" + " - id: detect-secrets\n" + " args: ['--foo', '--bar']\n" + ) + pre_commit_config_repo = RepositoryModels.PreCommitRepo( + repo="http://example-repo.com/", + rev="tag1", + hooks=[RepositoryModels.PreCommitHook(id="hook-id")], + ) + rev_info_mock = MagicMock(rev=pre_commit_config_repo.rev) + mock_hook_repo_rev_info.return_value = rev_info_mock + rev_info_mock.update.return_value = rev_info_mock + execute_result = pre_commit.autoupdate_hooks( + test_folder_path, force_update=False + ) + + assert execute_result.successful + + +def test_that_pre_commit_autoupdate_hooks_executes_successfully_when_not_forcing_updates_with_repos( + pre_commit: PreCommitAbstractionModels.PreCommitAbstraction, + mock_subprocess: MagicMock, +): + with ( + um.patch.object(Path, "exists") as mock_exists, + um.patch("builtins.open", um.mock_open()) as mock_open, + um.patch( + "secureli.modules.shared.abstractions.pre_commit.HookRepoRevInfo.from_config" + ) as mock_hook_repo_rev_info, + ): + mock_subprocess.run.return_value = CompletedProcess(args=[], returncode=0) + mock_exists.return_value = True + mock_open.return_value.read.return_value = ( + "repos:\n" + " - repo: my-repo\n" + " rev: tag1\n" + " hooks:\n" + " - id: detect-secrets\n" + " args: ['--foo', '--bar']\n" + ) + pre_commit_config_repo = RepositoryModels.PreCommitRepo( + repo="http://example-repo.com/", + rev="tag1", + hooks=[RepositoryModels.PreCommitHook(id="hook-id")], + ) + rev_info_mock = MagicMock(rev=pre_commit_config_repo.rev) + mock_hook_repo_rev_info.return_value = rev_info_mock + rev_info_mock.update.return_value = rev_info_mock + + execute_result = pre_commit.autoupdate_hooks( + test_folder_path, force_update=False, repos=[pre_commit_config_repo] + ) assert execute_result.successful - assert "--repo string" in mock_subprocess.run.call_args_list[0].args[0] ##### update ##### @@ -443,10 +550,14 @@ def test_check_for_hook_updates_infers_freeze_param_when_not_provided( pre_commit_config = RepositoryModels.PreCommitSettings( repos=[pre_commit_config_repo] ) - rev_info_mock = MagicMock(rev=pre_commit_config_repo.rev) + rev_info_mock = MagicMock( + rev=pre_commit_config_repo.rev, repo="http://example-repo.com/" + ) mock_hook_repo_rev_info.return_value = rev_info_mock rev_info_mock.update.return_value = rev_info_mock # Returning the same revision info on update means the hook will be considered up to date - pre_commit.check_for_hook_updates(pre_commit_config) + pre_commit.check_for_hook_updates( + pre_commit_config, + ) rev_info_mock.update.assert_called_with(tags_only=True, freeze=rev_is_sha) @@ -468,7 +579,9 @@ def test_check_for_hook_updates_respects_freeze_param_when_false( pre_commit_config = RepositoryModels.PreCommitSettings( repos=[pre_commit_config_repo] ) - rev_info_mock = MagicMock(rev=pre_commit_config_repo.rev) + rev_info_mock = MagicMock( + rev=pre_commit_config_repo.rev, repo="http://example-repo.com/" + ) mock_hook_repo_rev_info.return_value = rev_info_mock rev_info_mock.update.return_value = rev_info_mock # Returning the same revision info on update means the hook will be considered up to date pre_commit.check_for_hook_updates(pre_commit_config, freeze=False) @@ -489,7 +602,9 @@ def test_check_for_hook_updates_respects_freeze_param_when_true( pre_commit_config = RepositoryModels.PreCommitSettings( repos=[pre_commit_config_repo] ) - rev_info_mock = MagicMock(rev=pre_commit_config_repo.rev) + rev_info_mock = MagicMock( + rev=pre_commit_config_repo.rev, repo="http://example-repo.com/" + ) mock_hook_repo_rev_info.return_value = rev_info_mock rev_info_mock.update.return_value = rev_info_mock # Returning the same revision info on update means the hook will be considered up to date pre_commit.check_for_hook_updates(pre_commit_config, freeze=True) @@ -531,6 +646,50 @@ def test_check_for_hook_updates_returns_repos_with_new_revs( assert updated_repos[repo_urls[0]].newRev == "tag2" +def test_check_for_hook_updates_updates_repos_not_named_local( + pre_commit: PreCommitAbstractionModels.PreCommitAbstraction, +): + with um.patch( + "secureli.modules.shared.abstractions.pre_commit.HookRepoRevInfo.from_config" + ) as mock_hook_repo_rev_info: + pre_commit_config_repo = RepositoryModels.PreCommitRepo( + repo="local", + rev="tag1", + hooks=[RepositoryModels.PreCommitHook(id="hook-id")], + ) + pre_commit_config = RepositoryModels.PreCommitSettings( + repos=[pre_commit_config_repo] + ) + rev_info_mock = MagicMock( + rev=pre_commit_config_repo.rev, repo="http://example-repo.com/" + ) + mock_hook_repo_rev_info.return_value = rev_info_mock + rev_info_mock.update.return_value = rev_info_mock # Returning the same revision info on update means the hook will be considered up to date + pre_commit.check_for_hook_updates(pre_commit_config, freeze=True) + rev_info_mock.update.assert_called_with(tags_only=True, freeze=True) + + +def test_check_for_hook_updates_does_not_update_repos_named_local( + pre_commit: PreCommitAbstractionModels.PreCommitAbstraction, +): + with um.patch( + "secureli.modules.shared.abstractions.pre_commit.HookRepoRevInfo.from_config" + ) as mock_hook_repo_rev_info: + pre_commit_config_repo = RepositoryModels.PreCommitRepo( + repo="local", + rev="tag1", + hooks=[RepositoryModels.PreCommitHook(id="hook-id")], + ) + pre_commit_config = RepositoryModels.PreCommitSettings( + repos=[pre_commit_config_repo] + ) + rev_info_mock = MagicMock(rev=pre_commit_config_repo.rev, repo="local") + mock_hook_repo_rev_info.return_value = rev_info_mock + rev_info_mock.update.return_value = rev_info_mock # Returning the same revision info on update means the hook will be considered up to date + pre_commit.check_for_hook_updates(pre_commit_config, freeze=True) + rev_info_mock.update.assert_not_called() + + def test_pre_commit_config_exists( pre_commit: PreCommitAbstractionModels.PreCommitAbstraction, ):