By/allow writing pre commit output to files (#4697)

* fix logger setup on module init * changelog * precommit * ymlsplitter fix * changelog * changelog * write precommit results to files * changelog * fix json_output_path * update 'Werkzeug' version * remove python 3.9 support * using JSON_Handler * remove python 3.9 support * remove python 3.9 support * precommit * mypy * revert python 3.9 support removal * EOF * EOF * EOF * EOF * EOF * don't pre-install hooks * hook results in json * logs * revert json flag * run hooks serially * allow disabling multiprocessing * run hooks in multithreading * bring back install hooks * thread safety * disable multiprocessing * fix hooks prepare * hooks prepare fix * fix system hook preparation * adapt validate default_config.toml * organize imports * organize imports * test hook results to json * mypy fix * revet validate default config * mypy * mypy * localize json_handler * Update .changelog/4697.yml Co-authored-by: Yuval Hayun <[email protected]> * pr fixes * pre-commit fix * revert default_config.toml * revert default_config.toml * Update 4696.yml --------- Co-authored-by: Yuval Hayun <[email protected]>
demisto · Dec 15, 2024 · 1b8666d · 1b8666d
1 parent 6ad26b1
commit 1b8666d
Show file tree

Hide file tree

Showing 8 changed files with 148 additions and 33 deletions.
diff --git a/.changelog/4696.yml b/.changelog/4696.yml
@@ -0,0 +1,4 @@
+changes:
+- description: Fixed an issue where YmlSplitter attributes were being unintentionally updated.
+  type: fix
+pr_number: 4696
diff --git a/.changelog/4697.yml b/.changelog/4697.yml
@@ -0,0 +1,4 @@
+changes:
+- description: Added support for writing pre-commit results to files.
+  type: feature
+pr_number: 4697
diff --git a/demisto_sdk/commands/common/tools.py b/demisto_sdk/commands/common/tools.py
@@ -4644,3 +4644,10 @@ def filter_out_falsy_values(ls: Union[List, Tuple]) -> List:
         List filtered from None values.
     """
     return list(filter(lambda x: x, ls))
+
+
+def should_disable_multiprocessing():
+    disable_multiprocessing = os.getenv(
+        "DEMISTO_SDK_DISABLE_MULTIPROCESSING", "false"
+    ).lower() in ["true", "yes", "1"]
+    return disable_multiprocessing
diff --git a/demisto_sdk/commands/pre_commit/hooks/hook.py b/demisto_sdk/commands/pre_commit/hooks/hook.py
@@ -225,7 +225,7 @@ def safe_update_hook_args(hook: Dict, value: Any) -> None:
         None
     """
     args_key = "args"
-    if args_key in hook:
+    if args_key in hook and value not in hook[args_key]:
         hook[args_key].append(value)
     else:
         hook[args_key] = [value]
diff --git a/demisto_sdk/commands/pre_commit/hooks/system.py b/demisto_sdk/commands/pre_commit/hooks/system.py
@@ -20,6 +20,8 @@ def prepare_hook(self) -> GeneratedHooks:
         if "entry" in self.base_hook:
             entry = self.base_hook["entry"]
             bin_path = Path(sys.executable).parent
-            self.base_hook["entry"] = f"{bin_path}/{entry}"
+            self.base_hook["entry"] = (
+                f"{bin_path}/{entry}" if str(bin_path) not in entry else entry
+            )
         self.hooks.insert(self.hook_index, self.base_hook)
         return GeneratedHooks(hook_ids=[self.base_hook["id"]], parallel=self.parallel)
diff --git a/demisto_sdk/commands/pre_commit/pre_commit_command.py b/demisto_sdk/commands/pre_commit/pre_commit_command.py
@@ -22,10 +22,9 @@
 from demisto_sdk.commands.common.content_constant_paths import CONTENT_PATH, PYTHONPATH
 from demisto_sdk.commands.common.cpu_count import cpu_count
 from demisto_sdk.commands.common.git_util import GitUtil
+from demisto_sdk.commands.common.handlers import JSON_Handler
 from demisto_sdk.commands.common.logger import logger
-from demisto_sdk.commands.common.tools import (
-    write_dict,
-)
+from demisto_sdk.commands.common.tools import should_disable_multiprocessing, write_dict
 from demisto_sdk.commands.content_graph.commands.update import update_content_graph
 from demisto_sdk.commands.content_graph.interface import ContentGraphInterface
 from demisto_sdk.commands.content_graph.objects.base_content import BaseContent
@@ -118,6 +117,7 @@ def run_hook(
         precommit_env: dict,
         verbose: bool = False,
         stdout: Optional[int] = subprocess.PIPE,
+        json_output_path: Optional[Path] = None,
     ) -> int:
         """This function runs the pre-commit process and waits until finished.
         We run this function in multithread.
@@ -127,17 +127,23 @@ def run_hook(
             precommit_env (dict): The pre-commit environment variables
             verbose (bool, optional): Whether print verbose output. Defaults to False.
             stdout (Optional[int], optional): The way to handle stdout. Defaults to subprocess.PIPE.
-
+            json_output_path (Optional[Path]): Optional path to a JSON formatted output file/dir where pre-commit hooks
+                results are stored. None by deafult, and file is not created.
         Returns:
             int: return code - 0 if hook passed, 1 if failed
         """
         logger.debug(f"Running hook {hook_id}")
+
+        if json_output_path and json_output_path.is_dir():
+            json_output_path = json_output_path / f"{hook_id}.json"
+
         process = PreCommitRunner._run_pre_commit_process(
             PRECOMMIT_CONFIG_MAIN_PATH,
             precommit_env,
             verbose,
             stdout,
             command=["run", "-a", hook_id],
+            json_output_path=json_output_path,
         )
 
         if process.stdout:
@@ -153,6 +159,7 @@ def _run_pre_commit_process(
         verbose: bool,
         stdout=None,
         command: Optional[List[str]] = None,
+        json_output_path: Optional[Path] = None,
     ) -> subprocess.CompletedProcess:
         """Runs a process of pre-commit
 
@@ -162,13 +169,15 @@ def _run_pre_commit_process(
             verbose (bool): whether to print verbose output
             stdout (optional): use `subprocess.PIPE` to capture stdout. Use None to print it. Defaults to None.
             command (Optional[List[str]], optional): The pre-commit command to run. Defaults to None.
-
+            json_output_path (Optional[Path]): Optional path to a JSON formatted output file/dir where pre-commit hooks
+                results are stored. None by deafult, and file is not created.
         Returns:
             _type_: _description_
         """
         if command is None:
             command = ["run", "-a"]
-        return subprocess.run(
+        output = subprocess.PIPE if json_output_path else stdout
+        completed_process = subprocess.run(
             list(
                 filter(
                     None,
@@ -185,17 +194,25 @@ def _run_pre_commit_process(
             ),
             env=precommit_env,
             cwd=CONTENT_PATH,
-            stdout=stdout,
-            stderr=stdout,
+            stdout=output,
+            stderr=output,
             universal_newlines=True,
         )
+        # Only writing failed hook results.
+        if json_output_path and completed_process.returncode != 0:
+            with open(json_output_path, "w") as json_file:
+                json = JSON_Handler()
+                json.dump(completed_process.__dict__, json_file, indent=4)
+
+        return completed_process
 
     @staticmethod
     def run(
         pre_commit_context: PreCommitContext,
         precommit_env: dict,
         verbose: bool,
         show_diff_on_failure: bool,
+        json_output_path: Optional[Path] = None,
     ) -> int:
         """Execute the pre-commit hooks on the files.
 
@@ -204,7 +221,8 @@ def run(
             precommit_env (dict): The environment variables dict.
             verbose (bool):  Whether run pre-commit in verbose mode.
             show_diff_on_failure (bool): Whether to show diff when a hook fail or not.
-
+            json_output_path (Optional[Path]): Optional path to a JSON formatted output file/dir where pre-commit hooks
+                results are stored. None by deafult, and file is not created.
         Returns:
             int: The exit code - 0 if everything is valid.
         """
@@ -218,31 +236,47 @@ def run(
         write_dict(PRECOMMIT_CONFIG_MAIN_PATH, pre_commit_context.precommit_template)
         # we don't need the context anymore, we can clear it to free up memory for the pre-commit checks
         del pre_commit_context
+
         # install dependencies of all hooks in advance
         PreCommitRunner._run_pre_commit_process(
             PRECOMMIT_CONFIG_MAIN_PATH,
             precommit_env,
             verbose,
             command=["install-hooks"],
+            json_output_path=json_output_path
+            if not json_output_path or json_output_path.is_file()
+            else json_output_path / "install-hooks.json",
         )
 
         num_processes = cpu_count()
         all_hooks_exit_codes = []
-        for (
-            original_hook_id,
-            generated_hooks,
-        ) in PreCommitRunner.original_hook_id_to_generated_hook_ids.items():
+        hooks_to_run = PreCommitRunner.original_hook_id_to_generated_hook_ids.items()
+        logger.debug(f"run {hooks_to_run=}")
+
+        for original_hook_id, generated_hooks in hooks_to_run:
             if generated_hooks:
                 logger.debug(f"Running hook {original_hook_id} with {generated_hooks}")
                 hook_ids = generated_hooks.hook_ids
-                if generated_hooks.parallel and len(hook_ids) > 1:
+                if (
+                    generated_hooks.parallel
+                    and len(hook_ids) > 1
+                    and not should_disable_multiprocessing()
+                ):
+                    # We shall not write results to the same file if running hooks in parallel, therefore,
+                    # writing the results to a parallel directory.
+                    if json_output_path and not json_output_path.is_dir():
+                        json_output_path = (
+                            json_output_path.parent / json_output_path.stem
+                        )
+                        json_output_path.mkdir(exist_ok=True)
                     with ThreadPool(num_processes) as pool:
                         current_hooks_exit_codes = pool.map(
                             partial(
                                 PreCommitRunner.run_hook,
                                 precommit_env=precommit_env,
                                 verbose=verbose,
                                 stdout=subprocess.PIPE,
+                                json_output_path=json_output_path,
                             ),
                             hook_ids,
                         )
@@ -252,7 +286,7 @@ def run(
                             hook_id,
                             precommit_env=precommit_env,
                             verbose=verbose,
-                            stdout=None,
+                            json_output_path=json_output_path,
                         )
                         for hook_id in hook_ids
                     ]
@@ -284,6 +318,7 @@ def prepare_and_run(
         show_diff_on_failure: bool = False,
         exclude_files: Optional[Set[Path]] = None,
         dry_run: bool = False,
+        json_output_path: Optional[Path] = None,
     ) -> int:
         """Trigger the relevant hooks.
 
@@ -293,7 +328,8 @@ def prepare_and_run(
             show_diff_on_failure (bool, optional): Whether to show diff when a hook fail or not. Defaults to False.
             exclude_files (Optional[Set[Path]], optional): Files to exclude when running. Defaults to None.
             dry_run (bool, optional): Whether to run the pre-commit hooks in dry-run mode. Defaults to False.
-
+            json_output_path (Path, optional): Optional path to a JSON formatted output file/dir where pre-commit hooks
+                results are stored. None by default, and file is not created.
         Returns:
             int: The exit code, 0 if nothing failed.
         """
@@ -346,7 +382,11 @@ def prepare_and_run(
             )
             return ret_val
         ret_val = PreCommitRunner.run(
-            pre_commit_context, precommit_env, verbose, show_diff_on_failure
+            pre_commit_context,
+            precommit_env,
+            verbose,
+            show_diff_on_failure,
+            json_output_path,
         )
         return ret_val
 
@@ -399,13 +439,22 @@ def group_by_language(
     for integration_script_paths in more_itertools.chunked_even(
         integrations_scripts_mapping.keys(), INTEGRATIONS_BATCH
     ):
-        with multiprocessing.Pool(processes=cpu_count()) as pool:
-            content_items = pool.map(BaseContent.from_path, integration_script_paths)
-            for content_item in content_items:
-                if not content_item or not isinstance(content_item, IntegrationScript):
-                    continue
-                # content-item is a script/integration
+        if should_disable_multiprocessing():
+            # Run sequentially
+            content_items: List[Optional[BaseContent]] = list(
+                map(BaseContent.from_path, integration_script_paths)
+            )
+        else:
+            # Use multiprocessing (not supported when running within Content scripts/integrations).
+            with multiprocessing.Pool(processes=cpu_count()) as pool:
+                content_items = list(
+                    pool.map(BaseContent.from_path, integration_script_paths)
+                )
+
+        for content_item in content_items:
+            if isinstance(content_item, IntegrationScript):
                 integrations_scripts.add(content_item)
+
     logger.debug("Pre-Commit: Finished parsing all integrations and scripts")
     exclude_integration_script = set()
     for integration_script in integrations_scripts:
@@ -509,6 +558,7 @@ def pre_commit_manager(
     docker_image: Optional[str] = None,
     run_hook: Optional[str] = None,
     pre_commit_template_path: Optional[Path] = None,
+    json_output_path: Optional[Path] = None,
 ) -> int:
     """Run pre-commit hooks .
 
@@ -523,12 +573,15 @@ def pre_commit_manager(
         force_run_hooks (Optional[List[str]], optional): List for hooks to force run. Defaults to None.
         verbose (bool, optional): Whether run pre-commit in verbose mode. Defaults to False.
         show_diff_on_failure (bool, optional): Whether show git diff after pre-commit failure. Defaults to False.
-        dry_run (bool, optional): Whether to run the pre-commit hooks in dry-run mode, which will only create the config file.
+        dry_run (bool, optional): Whether to run the pre-commit hooks in dry-run mode, which will only create the
+            config file.
         run_docker_hooks (bool, optional): Whether to run docker based hooks or not.
         image_ref: (str, optional): Override the image from YAML / native config file with this image reference.
-        docker_image: (str, optional): Override the `docker_image` property in the template file. This is a comma separated list of: `from-yml`, `native:dev`, `native:ga`, `native:candidate`.
+        docker_image: (str, optional): Override the `docker_image` property in the template file. This is a comma
+            separated list of: `from-yml`, `native:dev`, `native:ga`, `native:candidate`.
         pre_commit_template_path (Path, optional): Path to the template pre-commit file.
-
+        json_output_path (Path, optional): Optional path to a JSON formatted output file/dir where pre-commit hooks results
+            are stored. None by default, and file is not created.
     Returns:
         int: Return code of pre-commit.
     """
@@ -597,6 +650,7 @@ def pre_commit_manager(
         show_diff_on_failure,
         exclude_files,
         dry_run,
+        json_output_path,
     )
 
 

diff --git a/demisto_sdk/commands/pre_commit/tests/pre_commit_test.py b/demisto_sdk/commands/pre_commit/tests/pre_commit_test.py
@@ -9,6 +9,7 @@
 import demisto_sdk.commands.pre_commit.pre_commit_command as pre_commit_command
 import demisto_sdk.commands.pre_commit.pre_commit_context as context
 from demisto_sdk.commands.common.handlers import DEFAULT_YAML_HANDLER as yaml
+from demisto_sdk.commands.common.handlers import JSON_Handler
 from demisto_sdk.commands.common.legacy_git_tools import git_path
 from demisto_sdk.commands.common.native_image import NativeImageConfig
 from demisto_sdk.commands.pre_commit.hooks.docker import DockerHook
@@ -21,6 +22,7 @@
     PreCommitContext,
     PreCommitRunner,
     group_by_language,
+    pre_commit_manager,
     preprocess_files,
     subprocess,
 )
@@ -801,3 +803,45 @@ def test_system_hooks():
         system_hook["repo"]["hooks"][0]["entry"]
         == f"{Path(sys.executable).parent}/demisto-sdk"
     )
+
+
+def test_run_pre_commit_with_json_output_path(mocker, tmp_path):
+    """
+    Given: A pre-commit setup with a specified JSON output path.
+    When: Running the pre-commit manager with a specific hook.
+    Then:
+        - The exit code is non-zero
+        - A JSON output file is created at the specified path
+    """
+    mocker.patch.object(pre_commit_command, "CONTENT_PATH", Path(tmp_path))
+
+    test_integration_path = (
+        tmp_path
+        / "Packs"
+        / "TestPack"
+        / "Integrations"
+        / "TestIntegration"
+        / "TestIntegration.yml"
+    )
+    test_integration_path.parent.mkdir(parents=True, exist_ok=True)
+
+    mocker.patch(
+        "demisto_sdk.commands.pre_commit.pre_commit_command.preprocess_files",
+        return_value=[test_integration_path],
+    )
+
+    exit_code = pre_commit_manager(
+        input_files=[test_integration_path],
+        run_hook="check-ast",
+        json_output_path=tmp_path,
+    )
+    hook_output_path = tmp_path / "check-ast.json"
+    assert exit_code != 0
+    assert hook_output_path.exists()
+    with open(hook_output_path, "r") as f:
+        json = JSON_Handler()
+        output = json.load(f)
+        assert 1 == output.get("returncode")
+        assert output.get("stdout").startswith(
+            "An error has occurred: FatalError: git failed. Is it installed, and are you in a Git repository directory?"
+        )
diff --git a/demisto_sdk/commands/validate/validators/IM_validators/IM101_image_too_large.py b/demisto_sdk/commands/validate/validators/IM_validators/IM101_image_too_large.py
@@ -34,21 +34,21 @@ def obtain_invalid_content_items(
                 content_object=content_item,
             )
             for content_item in content_items
-            if self.is_image_valid(content_item)
+            if content_item.image.exist and not self.is_image_valid(content_item)
         ]
 
     def is_image_valid(self, content_item: ContentTypes):
         file_type = content_item.image.file_path.suffix
         if file_type == ".png":
-            return content_item.image.get_file_size().st_size > IMAGE_MAX_SIZE
+            return not content_item.image.get_file_size().st_size > IMAGE_MAX_SIZE
 
         elif file_type == ".svg":
             # No size validation done for SVG images
-            return False
+            return True
 
         elif file_type == ".yml":
             image_size = int(((len(content_item.data["image"]) - 22) / 4) * 3)
-            return image_size > IMAGE_MAX_SIZE
+            return not image_size > IMAGE_MAX_SIZE
 
         # image can't be saved in a different file type
-        return True
+        return False