From f262cde177fd3050ee95e6308f26f396f9c5beb2 Mon Sep 17 00:00:00 2001 From: Jordan Heffernan <44213782+JordoHeffernan@users.noreply.github.com> Date: Tue, 26 Mar 2024 16:24:27 -0600 Subject: [PATCH] Feat: secureli 372 fix init when no pre-commit-config exists (#502) secureli-XXX in a final check on recent work, I noticed that init was failing if there was no .pre-commit-config.yaml. This should fix that issue, maintain new functionality for copying existing yaml settings, and update tests. ## Changes * Updated action, language_support, and pre_commit to account for a lack of a .pre-commit-config.yaml by creating blank one if it doesn't exist. If it does exist and isn't where we want it, we move it. ## Testing * all tests pass * in a dummy repo, run secureli init _with_ a .pre-commit-config.yaml in the root directory * in a dummy repo, run secureli init _without_ a .pre-commit-config.yaml in the root directory ## Clean Code Checklist - [x] Meets acceptance criteria for issue - [x] New logic is covered with automated tests - [ ] Appropriate exception handling added - [ ] Thoughtful logging included - [ ] Documentation is updated - [ ] Follow-up work is documented in TODOs - [x] TODOs have a ticket associated with them - [x] No commented-out code included --------- Co-authored-by: Jordan Heffernan --- secureli/actions/action.py | 35 ++++++++++---- .../language_analyzer/language_support.py | 9 ++-- .../modules/shared/abstractions/pre_commit.py | 14 +++++- tests/actions/test_action.py | 46 ++++++++++++------- tests/end-to-end/testinstallnewhooks.bats | 22 +++++++++ tests/end-to-end/testpreservehooks.bats | 3 +- .../shared/abstractions/test_pre_commit.py | 12 +++++ 7 files changed, 109 insertions(+), 32 deletions(-) create mode 100644 tests/end-to-end/testinstallnewhooks.bats diff --git a/secureli/actions/action.py b/secureli/actions/action.py index e4c7128b..c89b8d3e 100644 --- a/secureli/actions/action.py +++ b/secureli/actions/action.py @@ -55,7 +55,11 @@ def get_secureli_config(self, reset: bool) -> secureli_config.SecureliConfig: ) def verify_install( - self, folder_path: Path, reset: bool, always_yes: bool, files: list[Path] + self, + folder_path: Path, + reset: bool, + always_yes: bool, + files: list[Path], ) -> VerifyResult: """ Installs, upgrades or verifies the current seCureLI installation @@ -75,22 +79,32 @@ def verify_install( return VerifyResult( outcome=update_config.outcome, ) - - pre_commit_config_location = self.action_deps.hooks_scanner.pre_commit.get_preferred_pre_commit_config_path( + pre_commit_config_location_is_correct = self.action_deps.hooks_scanner.pre_commit.get_pre_commit_config_path_is_correct( + folder_path + ) + preferred_config_path = self.action_deps.hooks_scanner.pre_commit.get_preferred_pre_commit_config_path( folder_path ) - if not pre_commit_config_location.exists(): + pre_commit_to_preserve = ( + self.action_deps.hooks_scanner.pre_commit.pre_commit_config_exists( + folder_path + ) + and not pre_commit_config_location_is_correct + ) + if pre_commit_to_preserve: update_result: VerifyResult = ( self._update_secureli_pre_commit_config_location( folder_path, always_yes ) ) - pre_commit_config_location = update_result.file_path + if update_result.outcome != VerifyOutcome.UPDATE_SUCCEEDED: self.action_deps.echo.error( - "seCureLI pre-commit-config.yaml could not be updated." + "seCureLI .pre-commit-config.yaml could not be moved." ) return update_result + else: + preferred_config_path = update_result.file_path config = self.get_secureli_config(reset=reset) languages = [] @@ -126,7 +140,7 @@ def verify_install( languages, newly_detected_languages, always_yes, - pre_commit_config_location, + preferred_config_path if pre_commit_to_preserve else None, ) else: self.action_deps.echo.print( @@ -159,7 +173,6 @@ def _install_secureli( # pre-install new_install = len(detected_languages) == len(install_languages) - should_install = self._prompt_to_install( install_languages, always_yes, new_install ) @@ -416,7 +429,7 @@ def _update_secureli_pre_commit_config_location( to avoid breaking backward compatibility. """ self.action_deps.echo.print( - "seCureLI's .pre-commit-config.yaml is in a deprecated location." + "The .pre-commit-config.yaml is in a deprecated location." ) response = always_yes or self.action_deps.echo.confirm( "Would you like it automatically moved to the .secureli/ directory?", @@ -437,7 +450,9 @@ def _update_secureli_pre_commit_config_location( else: self.action_deps.echo.warning(".pre-commit-config.yaml migration declined") deprecated_location = ( - self.action_deps.hooks_scanner.get_pre_commit_config_path(folder_path) + self.action_deps.hooks_scanner.pre_commit.get_pre_commit_config_path( + folder_path + ) ) return VerifyResult( outcome=VerifyOutcome.UPDATE_CANCELED, file_path=deprecated_location diff --git a/secureli/modules/language_analyzer/language_support.py b/secureli/modules/language_analyzer/language_support.py index 1b982f46..c6ce8d9d 100644 --- a/secureli/modules/language_analyzer/language_support.py +++ b/secureli/modules/language_analyzer/language_support.py @@ -48,8 +48,10 @@ def apply_support( as well as a secret-detection hook ID, if present. """ - path_to_pre_commit_file: Path = self.pre_commit_hook.get_pre_commit_config_path( - SecureliConfig.FOLDER_PATH + path_to_pre_commit_file: Path = ( + self.pre_commit_hook.get_preferred_pre_commit_config_path( + SecureliConfig.FOLDER_PATH + ) ) linter_config_write_result = self._write_pre_commit_configs( @@ -156,7 +158,8 @@ def build_pre_commit_config( try: data = yaml.safe_load(stream) existing_data = data or {} - config_repos += data["repos"] + config_repos += data["repos"] if data and data.get("repos") else [] + except yaml.YAMLError: self.echo.error( f"There was an issue parsing existing pre-commit-config.yaml." diff --git a/secureli/modules/shared/abstractions/pre_commit.py b/secureli/modules/shared/abstractions/pre_commit.py index 964d3c65..5aa1eb10 100644 --- a/secureli/modules/shared/abstractions/pre_commit.py +++ b/secureli/modules/shared/abstractions/pre_commit.py @@ -344,6 +344,18 @@ def get_pre_commit_config_path(self, folder_path: Path) -> Path: f"Could not find pre-commit hooks in .secureli/{self.CONFIG_FILE_NAME}" ) + def get_pre_commit_config_path_is_correct(self, folder_path: Path) -> bool: + """Returns whether a pre-commit-config exists in a given folder path""" + preferred_pre_commit_config_location = ( + self.get_preferred_pre_commit_config_path(folder_path) + ) + pre_commit_config_path = folder_path / self.CONFIG_FILE_NAME + return ( + preferred_pre_commit_config_location.exists() + or pre_commit_config_path.exists() + and pre_commit_config_path == preferred_pre_commit_config_location + ) + def get_pre_commit_config(self, folder_path: Path): """ Gets the contents of the .pre-commit-config file and returns it as a dictionary @@ -366,7 +378,7 @@ def migrate_config_file(self, folder_path): Feel free to delete this method after an appropriate period of time (a few months?) """ existing_config_file_path = self.get_pre_commit_config_path(folder_path) - new_config_file_path = folder_path / ".secureli" / self.CONFIG_FILE_NAME + new_config_file_path = self.get_preferred_pre_commit_config_path(folder_path) self.echo.print( f"Moving {existing_config_file_path} to {new_config_file_path}..." ) diff --git a/tests/actions/test_action.py b/tests/actions/test_action.py index c9c6faf1..83d575de 100644 --- a/tests/actions/test_action.py +++ b/tests/actions/test_action.py @@ -1,5 +1,5 @@ from pathlib import Path -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, patch, mock_open import pytest from secureli.modules.shared.abstractions.pre_commit import InstallResult @@ -181,12 +181,18 @@ def test_that_initialize_repo_install_flow_warns_about_skipped_files( def test_that_initialize_repo_can_be_canceled( action: Action, mock_echo: MagicMock, + mock_hooks_scanner: MagicMock, ): mock_echo.confirm.return_value = False + mock_hooks_scanner.pre_commit.get_pre_commit_config_path_is_correct.return_value = ( + True + ) + with (patch.object(Path, "exists", return_value=True),): + action.verify_install( + test_folder_path, reset=True, always_yes=False, files=None + ) - action.verify_install(test_folder_path, reset=True, always_yes=False, files=None) - - mock_echo.error.assert_called_with("User canceled install process") + mock_echo.error.assert_called_with("User canceled install process") def test_that_initialize_repo_selects_previously_selected_language( @@ -411,7 +417,7 @@ def test_that_verify_install_returns_success_result_newly_detected_language_inst assert verify_result.outcome == VerifyOutcome.INSTALL_SUCCEEDED -def test_that_verify_install_returns_failure_result_without_re_commit_config_file_path( +def test_that_verify_install_returns_failure_result_without_pre_commit_config_file_path( action: Action, mock_hooks_scanner: MagicMock, mock_echo: MagicMock, @@ -420,14 +426,18 @@ def test_that_verify_install_returns_failure_result_without_re_commit_config_fil mock_hooks_scanner.pre_commit.get_preferred_pre_commit_config_path.return_value = ( test_folder_path / ".secureli" / ".pre-commit-config.yaml" ) + mock_hooks_scanner.pre_commit.get_pre_commit_config_path_is_correct.return_value = ( + False + ) mock_hooks_scanner.pre_commit.migrate_config_file.side_effect = Exception( "ERROR" ) + verify_result = action.verify_install( test_folder_path, reset=False, always_yes=True, files=None ) mock_echo.error.assert_called_once_with( - "seCureLI pre-commit-config.yaml could not be updated." + "seCureLI .pre-commit-config.yaml could not be moved." ) assert verify_result.outcome == VerifyOutcome.UPDATE_FAILED @@ -437,10 +447,13 @@ def test_that_verify_install_continues_after_pre_commit_config_file_moved( mock_hooks_scanner: MagicMock, mock_echo: MagicMock, ): - with (patch.object(Path, "exists", return_value=False),): + with (patch.object(Path, "exists", return_value=True),): mock_hooks_scanner.pre_commit.get_preferred_pre_commit_config_path.return_value = ( test_folder_path / ".secureli" / ".pre-commit-config.yaml" ) + mock_hooks_scanner.pre_commit.get_pre_commit_config_path_is_correct.return_value = ( + False + ) verify_result = action.verify_install( test_folder_path, reset=False, always_yes=True, files=None ) @@ -457,26 +470,27 @@ def test_that_update_secureli_pre_commit_config_location_moves_file( update_file_location, True ) mock_echo.print.assert_called_once_with( - "seCureLI's .pre-commit-config.yaml is in a deprecated location." + "The .pre-commit-config.yaml is in a deprecated location." ) mock_hooks_scanner.pre_commit.migrate_config_file.assert_called_with( update_file_location ) assert update_result.outcome == VerifyOutcome.UPDATE_SUCCEEDED + # assert update_result.file_path == update_file_location def test_that_update_secureli_pre_commit_config_fails_on_exception( action: Action, mock_hooks_scanner: MagicMock, ): - with pytest.raises(Exception): - mock_hooks_scanner.pre_commit.get_preferred_pre_commit_config_path.return_value = ( - test_folder_path / ".secureli" / ".pre-commit-config.yaml" - ) - update_result = action._update_secureli_pre_commit_config_location( - test_folder_path, True - ) - assert update_result.outcome == VerifyOutcome.UPDATE_FAILED + mock_hooks_scanner.pre_commit.migrate_config_file.side_effect = Exception("ERROR") + mock_hooks_scanner.pre_commit.get_preferred_pre_commit_config_path.return_value = ( + test_folder_path / ".secureli" / ".pre-commit-config.yaml" + ) + update_result = action._update_secureli_pre_commit_config_location( + test_folder_path, True + ) + assert update_result.outcome == VerifyOutcome.UPDATE_FAILED def test_that_update_secureli_pre_commit_config_location_cancels_on_user_response( diff --git a/tests/end-to-end/testinstallnewhooks.bats b/tests/end-to-end/testinstallnewhooks.bats new file mode 100644 index 00000000..12bf185b --- /dev/null +++ b/tests/end-to-end/testinstallnewhooks.bats @@ -0,0 +1,22 @@ +# Test to ensure pre-exisiting hooks within the .pre-commit-config.yaml files +# are persisted when installing secureli + +MOCK_REPO='tests/test-data/mock-repo' + +setup() { + load "${BATS_LIBS_ROOT}/bats-support/load" + load "${BATS_LIBS_ROOT}/bats-assert/load" + mkdir -p $MOCK_REPO + echo 'print("hello world!")' > $MOCK_REPO/hw.py + run git init $MOCK_REPO +} + +@test "can preserve pre-existing hooks" { + run python secureli/main.py init -y --directory $MOCK_REPO + run grep 'https://github.com/psf/black' $MOCK_REPO/.secureli/.pre-commit-config.yaml + assert_output --partial 'https://github.com/psf/black' +} + +teardown() { + rm -rf $MOCK_REPO +} diff --git a/tests/end-to-end/testpreservehooks.bats b/tests/end-to-end/testpreservehooks.bats index 13653bd8..30d653e4 100644 --- a/tests/end-to-end/testpreservehooks.bats +++ b/tests/end-to-end/testpreservehooks.bats @@ -7,10 +7,9 @@ setup() { load "${BATS_LIBS_ROOT}/bats-support/load" load "${BATS_LIBS_ROOT}/bats-assert/load" mkdir -p $MOCK_REPO - echo '# Existing YAML Contents should persist' > $MOCK_REPO/.pre-commit-config.yaml echo 'repos:' >> $MOCK_REPO/.pre-commit-config.yaml echo '- repo: https://github.com/hhatto/autopep8' >> $MOCK_REPO/.pre-commit-config.yaml - echo ' rev: v2.0.4' >> $MOCK_REPO/.pre-commit-config.yaml + echo ' rev: v2.1.0' >> $MOCK_REPO/.pre-commit-config.yaml echo ' hooks:' >> $MOCK_REPO/.pre-commit-config.yaml echo ' - id: autopep8' >> $MOCK_REPO/.pre-commit-config.yaml echo 'fail_fast: false' >> $MOCK_REPO/.pre-commit-config.yaml diff --git a/tests/modules/shared/abstractions/test_pre_commit.py b/tests/modules/shared/abstractions/test_pre_commit.py index 4c05d777..2dc712bf 100644 --- a/tests/modules/shared/abstractions/test_pre_commit.py +++ b/tests/modules/shared/abstractions/test_pre_commit.py @@ -571,3 +571,15 @@ def test_migrate_config_file_moves_pre_commit_conig( f"Moving {old_location} to {new_location}..." ) mock_move.assert_called_once() + + +def test_get_pre_commit_config_path_is_correct_returns_expected_values( + pre_commit: PreCommitAbstraction, +): + bad_result = pre_commit.get_pre_commit_config_path_is_correct(test_folder_path) + assert bad_result == False + with (um.patch.object(Path, "exists", return_value=True),): + good_result = pre_commit.get_pre_commit_config_path_is_correct( + test_folder_path / ".secureli" + ) + assert good_result == True