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 <[email protected]>
slalombuild · Mar 26, 2024 · f262cde · f262cde
1 parent f7708ef
commit f262cde
Show file tree

Hide file tree

Showing 7 changed files with 109 additions and 32 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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