diff --git a/.changelog/4175.yml b/.changelog/4175.yml new file mode 100644 index 00000000000..b9c3119fd94 --- /dev/null +++ b/.changelog/4175.yml @@ -0,0 +1,4 @@ +changes: +- description: Added handling for API Modules in **pre-commit**. + type: feature +pr_number: 4175 diff --git a/TestSuite/repo.py b/TestSuite/repo.py index 8b372fb6c2d..c1504950de6 100644 --- a/TestSuite/repo.py +++ b/TestSuite/repo.py @@ -332,6 +332,7 @@ def init_git(self): self.git_util = GitUtil(self.path) self.git_util.commit_files("Initial Commit") self.git_util.repo.create_head(DEMISTO_GIT_PRIMARY_BRANCH) + self.git_util.repo.create_remote("origin", "dummy_url") def working_dir(self): return self.path diff --git a/demisto_sdk/commands/common/git_util.py b/demisto_sdk/commands/common/git_util.py index b4e8727cdbb..e8633380b17 100644 --- a/demisto_sdk/commands/common/git_util.py +++ b/demisto_sdk/commands/common/git_util.py @@ -712,16 +712,8 @@ def _get_all_changed_files(self, prev_ver: Optional[str] = None) -> Set[Path]: Returns: Set[Path]: of Paths to files changed in the current branch. """ - try: - self.fetch() - - except Exception as e: - logger.warning( - f"Failed to fetch branch '{self.get_current_working_branch()}' " - f"from remote '{self.repo.remote().name}' ({self.repo.remote().url}). Continuing without fetching." - ) - logger.debug(f"Error: {e}") + self.fetch() remote, branch = self.handle_prev_ver(prev_ver) current_hash = self.get_current_commit_hash() @@ -1062,7 +1054,14 @@ def _is_file_git_ignored(self, file_path: str) -> bool: return bool(self.repo.ignored(file_path)) def fetch(self): - self.repo.remote(DEMISTO_GIT_UPSTREAM).fetch() + try: + self.repo.remote(DEMISTO_GIT_UPSTREAM).fetch() + except Exception as e: + logger.warning( + f"Failed to fetch branch '{self.get_current_working_branch()}' " + f"from remote '{self.repo.remote().name}' ({self.repo.remote().url}). Continuing without fetching." + ) + logger.debug(f"Error: {e}") def fetch_all(self): for remote in self.repo.remotes: diff --git a/demisto_sdk/commands/content_graph/objects/base_script.py b/demisto_sdk/commands/content_graph/objects/base_script.py index fb54f447b32..65c39d47b32 100644 --- a/demisto_sdk/commands/content_graph/objects/base_script.py +++ b/demisto_sdk/commands/content_graph/objects/base_script.py @@ -15,8 +15,8 @@ ContentType, RelationshipType, ) -from demisto_sdk.commands.content_graph.objects.base_content import ( - BaseNode, +from demisto_sdk.commands.content_graph.objects.integration import ( + Integration, ) from demisto_sdk.commands.content_graph.objects.integration_script import ( Argument, @@ -64,12 +64,12 @@ def prepare_for_upload( return data @property - def imported_by(self) -> List[BaseNode]: + def imported_by(self) -> List[Integration]: return [ - r.content_item_to + r.content_item_to # type: ignore[misc] for r in self.relationships_data[RelationshipType.IMPORTS] if r.content_item_to.database_id == r.source_id - ] + ] # type: ignore[return-value] def dump(self, dir: DirectoryPath, marketplace: MarketplaceVersions) -> None: dir.mkdir(exist_ok=True, parents=True) diff --git a/demisto_sdk/commands/pre_commit/hooks/docker.py b/demisto_sdk/commands/pre_commit/hooks/docker.py index 51b6bcd98f9..9cbf4f1f560 100644 --- a/demisto_sdk/commands/pre_commit/hooks/docker.py +++ b/demisto_sdk/commands/pre_commit/hooks/docker.py @@ -37,6 +37,7 @@ from demisto_sdk.commands.pre_commit.hooks.hook import Hook NO_SPLIT = None +USER_DEMITSO = "demisto" @lru_cache() @@ -342,7 +343,7 @@ def generate_hooks( docker_extra_args = self._get_property("pass_docker_extra_args", "") new_hook[ "entry" - ] = f'--entrypoint {new_hook.get("entry")} {docker_extra_args} {get_environment_flag(env)} {"--quiet" if quiet else ""} {dev_image}' + ] = f'--entrypoint {new_hook.get("entry")} {docker_extra_args} --user {USER_DEMITSO} {get_environment_flag(env)} {"--quiet" if quiet else ""} {dev_image}' ret_hooks = [] for ( integration_script, diff --git a/demisto_sdk/commands/pre_commit/pre_commit_command.py b/demisto_sdk/commands/pre_commit/pre_commit_command.py index 0131f49a55c..f963a47797b 100644 --- a/demisto_sdk/commands/pre_commit/pre_commit_command.py +++ b/demisto_sdk/commands/pre_commit/pre_commit_command.py @@ -13,6 +13,7 @@ from packaging.version import Version from demisto_sdk.commands.common.constants import ( + API_MODULES_PACK, DEFAULT_PYTHON_VERSION, INTEGRATIONS_DIR, PACKS_FOLDER, @@ -25,10 +26,13 @@ from demisto_sdk.commands.common.tools import ( 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 from demisto_sdk.commands.content_graph.objects.integration_script import ( IntegrationScript, ) +from demisto_sdk.commands.content_graph.objects.script import Script from demisto_sdk.commands.pre_commit.hooks.docker import DockerHook from demisto_sdk.commands.pre_commit.hooks.hook import Hook, join_files from demisto_sdk.commands.pre_commit.hooks.mypy import MypyHook @@ -323,6 +327,7 @@ def group_by_language( """ integrations_scripts_mapping = defaultdict(set) infra_files = [] + api_modules = [] for file in files: if file.is_dir(): continue @@ -347,19 +352,50 @@ def group_by_language( infra_files.append(file) language_to_files: Dict[str, Set] = defaultdict(set) - integrations_scripts = [] + integrations_scripts: Set[IntegrationScript] = set() for integration_script_paths in more_itertools.chunked_even( integrations_scripts_mapping.keys(), INTEGRATIONS_BATCH ): with multiprocessing.Pool(processes=cpu_count()) as pool: - integrations_scripts.extend( - pool.map(BaseContent.from_path, integration_script_paths) - ) + 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 + integrations_scripts.add(content_item) exclude_integration_script = set() for integration_script in integrations_scripts: - if not integration_script or not isinstance( - integration_script, IntegrationScript - ): + if (pack := integration_script.in_pack) and pack.object_id == API_MODULES_PACK: + # add api modules to the api_modules list, we will handle them later + api_modules.append(integration_script) + continue + if api_modules: + with ContentGraphInterface() as graph: + update_content_graph(graph) + api_modules: List[Script] = graph.search( # type: ignore[no-redef] + object_id=[api_module.object_id for api_module in api_modules] + ) + for api_module in api_modules: + assert isinstance(api_module, Script) + for imported_by in api_module.imported_by: + # we need to add the api module for each integration that uses it, so it will execute the api module check + integrations_scripts.add(imported_by) + integrations_scripts_mapping[imported_by.path.parent].update( + add_related_files( + api_module.path + if not api_module.path.is_absolute() + else api_module.path.relative_to(CONTENT_PATH) + ) + | add_related_files( + imported_by.path + if not imported_by.path.is_absolute() + else imported_by.path.relative_to(CONTENT_PATH) + ) + ) + + for integration_script in integrations_scripts: + if (pack := integration_script.in_pack) and pack.object_id == API_MODULES_PACK: + # we dont need to lint them individually, they will be run with the integrations that uses them continue if integration_script.deprecated: # we exclude deprecate integrations and scripts from pre-commit. @@ -385,7 +421,14 @@ def group_by_language( (path, integration_script) for path in integrations_scripts_mapping[code_file_path] }, - {(integration_script.path.relative_to(CONTENT_PATH), integration_script)}, + { + ( + integration_script.path.relative_to(CONTENT_PATH) + if integration_script.path.is_absolute() + else integration_script.path, + integration_script, + ) + }, ) if infra_files: @@ -488,6 +531,33 @@ def pre_commit_manager( ) +def add_related_files(file: Path) -> Set[Path]: + """This returns the related files set, including the original file + If the file is `.yml`, it will add the `.py` file and the test file. + If the file is `.py` or `.ps1`, it will add the tests file. + + Args: + file (Path): The file to add related files for. + + Returns: + Set[Path]: The set of related files. + """ + files_to_run = set() + files_to_run.add(file) + if ".yml" in (file.suffix for file in files_to_run): + py_file_path = file.with_suffix(".py") + if py_file_path.exists(): + files_to_run.add(py_file_path) + if {".py", ".ps1"}.intersection({file.suffix for file in files_to_run}): + if ".py" in (file.suffix for file in files_to_run): + test_file = file.with_name(f"{file.stem}_test.py") + else: + test_file = file.with_name(f"{file.stem}.Tests.ps1") + if test_file.exists(): + files_to_run.add(test_file) + return files_to_run + + def preprocess_files( input_files: Optional[Iterable[Path]] = None, staged_only: bool = False, @@ -518,20 +588,7 @@ def preprocess_files( if file.is_dir(): files_to_run.update({path for path in file.rglob("*") if path.is_file()}) else: - files_to_run.add(file) - # If the current file is a yml file, add the matching python file to files_to_run - if file.suffix == ".yml": - py_file_path = file.with_suffix(".py") - if py_file_path.exists(): - files_to_run.add(py_file_path) - if file.suffix in (".py", ".ps1"): - if file.suffix == ".py": - test_file = file.with_name(f"{file.stem}_test.py") - else: - test_file = file.with_name(f"{file.stem}.Tests.ps1") - if test_file.exists(): - files_to_run.add(test_file) - + files_to_run.update(add_related_files(file)) # convert to relative file to content path relative_paths = { file.relative_to(CONTENT_PATH) if file.is_absolute() else file diff --git a/demisto_sdk/commands/pre_commit/tests/pre_commit_test.py b/demisto_sdk/commands/pre_commit/tests/pre_commit_test.py index 4dbc31802d3..66e28cda2d9 100644 --- a/demisto_sdk/commands/pre_commit/tests/pre_commit_test.py +++ b/demisto_sdk/commands/pre_commit/tests/pre_commit_test.py @@ -25,6 +25,7 @@ subprocess, ) from TestSuite.repo import Repo +from TestSuite.test_tools import ChangeCWD TEST_DATA_PATH = ( Path(git_path()) / "demisto_sdk" / "commands" / "pre_commit" / "tests" / "test_data" @@ -195,6 +196,41 @@ def test_config_files(mocker, repo: Repo): assert (Path(repo.path) / ".pre-commit-config-needs.yaml").exists() +def test_handle_api_modules(mocker, git_repo: Repo): + """ + Given: + - A repository with a pack that contains an API module and a pack that contains an integration that uses the API module + + When: + - Running demisto-sdk pre-commit + + Then: + - Ensure that the API module is added to the files to run + - Ensure that the integration that uses the API module is added to the files to run, both related to the *integration* + """ + pack1 = git_repo.create_pack("ApiModules") + script = pack1.create_script("TestApiModule") + pack2 = git_repo.create_pack("Pack2") + integration = pack2.create_integration( + "integration1", code="from TestApiModule import *" + ) + mocker.patch.object(pre_commit_command, "CONTENT_PATH", Path(git_repo.path)) + with ChangeCWD(git_repo.path): + git_repo.create_graph() + files_to_run = group_by_language( + {Path(script.yml.path).relative_to(git_repo.path)} + ) + files_to_run = {(path, obj.path) for path, obj in files_to_run[0]["2.7"]} + assert ( + Path(script.yml.path).relative_to(git_repo.path), + integration.object.path.relative_to(git_repo.path), + ) in files_to_run + assert ( + Path(integration.yml.path).relative_to(git_repo.path), + integration.object.path.relative_to(git_repo.path), + ) in files_to_run + + def test_mypy_hooks(mocker): """ Testing mypy hook created successfully (the python version is correct) @@ -355,6 +391,7 @@ def test_preprocess_files_with_input_yml_files(self, mocker, repo): expected_output = { Path(integration.yml.rel_path), Path(integration.code.rel_path), + Path(integration.test.rel_path), } mocker.patch.object(GitUtil, "get_all_files", return_value=relative_paths) output = preprocess_files(input_files=input_files)