[Contributions] Update untracked files collection in external PR usec…

…ase (#4394) * Update pre-commit to use abspath to collect untracked files * Update validate to use abspath to collect untracked files * Update log message for collected untracked files * Format * Add get_untracked_files_in_content helper function * Add get_untracked_files_in_content helper function in validate * Format * Add release notes * RN description refactor * Updated file comparisons to use Path objects * Format * Updated file path comparison to use comprehension * Implement custom is_relative_to function to support python 3.8 * Format
demisto · Jul 3, 2024 · c9f6ab0 · c9f6ab0
1 parent 636eaf1
commit c9f6ab0
Show file tree

Hide file tree

Showing 5 changed files with 116 additions and 23 deletions.
diff --git a/.changelog/4394.yml b/.changelog/4394.yml
@@ -0,0 +1,4 @@
+changes:
+- description: Fixed an issue where *pre-commit* and *validate* commands collected invalid untracked files when running on contribution PR.
+  type: fix
+pr_number: 4394
diff --git a/demisto_sdk/commands/pre_commit/pre_commit_command.py b/demisto_sdk/commands/pre_commit/pre_commit_command.py
@@ -669,17 +669,12 @@ def preprocess_files(
             The following code segment retrieves all relevant untracked file paths that were changed in the external contribution PR
             and adds them to `raw_files`. See CIAC-10490 for more info.
             """
-            # filter out a string list of untracked files with a path that starts with "Packs/"
-            # The file paths in the build machine are relative.
-            untracked_files_list = filter(
-                lambda f: f.startswith("Packs/"), git_util.repo.untracked_files
-            )
             logger.info(
-                f"\n[cyan]Running on untracked files: {untracked_files_list}[/cyan]"
+                "\n[cyan]CONTRIB_BRANCH variable found, trying to collected changed untracked files from external contribution PR[/cyan]"
             )
-            # convert the string list of untracked files to a set of Path object
-            untracked_files_paths = set(map(Path, untracked_files_list))
-            raw_files = raw_files.union(untracked_files_paths)
+            valid_untracked_files_paths = get_untracked_files_in_content(git_util)
+            raw_files = raw_files.union(valid_untracked_files_paths)
+            logger.info(f"\n######## - Running on collected files:\n{raw_files}")
     elif all_files:
         raw_files = all_git_files
     else:
@@ -700,3 +695,30 @@ def preprocess_files(
     }
     # filter out files that are not in the content git repo (e.g in .gitignore)
     return relative_paths & all_git_files
+
+
+def get_untracked_files_in_content(git_util) -> Set[Path]:
+    """
+    Filter out a string list of untracked files with a path thats inside the build machine's content repository.
+    The file paths in the build machine are relative so we use absolute path (resolve) to make sure the files are in content.
+    """
+    untracked_files_paths = {
+        Path(f)
+        for f in git_util.repo.untracked_files
+        if is_relative_to(path=Path(f).resolve(), base=CONTENT_PATH)
+    }
+    logger.info(f"\n######## - Modified untracked:\n{untracked_files_paths}")
+    return untracked_files_paths
+
+
+def is_relative_to(path: Path, base: Path) -> bool:
+    """
+    The Path class in Python's pathlib module got the native is_relative_to method starting
+    from Python 3.9, so it fails on python 3.8.
+    This is a custom implementation for the is_relative_to method.
+    """
+    try:
+        path.relative_to(base)
+        return True
+    except ValueError:
+        return False
diff --git a/demisto_sdk/commands/pre_commit/tests/pre_commit_test.py b/demisto_sdk/commands/pre_commit/tests/pre_commit_test.py
@@ -483,11 +483,12 @@ def test_preprocess_files_with_all_files(self, mocker):
         assert output == expected_output
 
     @pytest.mark.parametrize(
-        "untracked_files, modified_files, expected_output",
+        "untracked_files, modified_files, untracked_files_in_content ,expected_output",
         [
             (
                 ["Packs/untracked.txt"],
                 set([Path("Packs/modified.txt")]),
+                set([Path("Packs/untracked.txt")]),
                 set([Path("Packs/modified.txt"), Path("Packs/untracked.txt")]),
             ),
             (
@@ -498,6 +499,12 @@ def test_preprocess_files_with_all_files(self, mocker):
                     "another/invalid/path/untracked.txt",
                 ],
                 set([Path("Packs/modified.txt")]),
+                set(
+                    [
+                        Path("Packs/untracked_1.txt"),
+                        Path("Packs/untracked_2.txt"),
+                    ]
+                ),
                 set(
                     [
                         Path("Packs/modified.txt"),
@@ -520,6 +527,12 @@ def test_preprocess_files_with_all_files(self, mocker):
                         Path("Packs/untracked_2.txt"),
                     ]
                 ),
+                set(
+                    [
+                        Path("Packs/untracked_1.txt"),
+                        Path("Packs/untracked_2.txt"),
+                    ]
+                ),
             ),
         ],
         ids=[
@@ -529,7 +542,12 @@ def test_preprocess_files_with_all_files(self, mocker):
         ],
     )
     def test_preprocess_files_in_external_pr_use_case(
-        self, mocker, untracked_files, modified_files, expected_output
+        self,
+        mocker,
+        untracked_files,
+        modified_files,
+        untracked_files_in_content,
+        expected_output,
     ):
         """
         This UT verifies changes made to pre commit command to support collection of
@@ -558,6 +576,12 @@ def test_preprocess_files_in_external_pr_use_case(
             "git.repo.base.Repo._get_untracked_files",
             return_value=untracked_files,
         )
+        mocker.patch.object(
+            pre_commit_command,
+            "get_untracked_files_in_content",
+            return_value=untracked_files_in_content,
+        )
+
         output = preprocess_files(use_git=True)
         assert output == expected_output
 

diff --git a/demisto_sdk/commands/validate/initializer.py b/demisto_sdk/commands/validate/initializer.py
@@ -25,6 +25,7 @@
     PathLevel,
 )
 from demisto_sdk.commands.common.content import Content
+from demisto_sdk.commands.common.content_constant_paths import CONTENT_PATH
 from demisto_sdk.commands.common.logger import logger
 from demisto_sdk.commands.common.tools import (
     detect_file_level,
@@ -255,19 +256,39 @@ def get_unfiltered_changed_files_from_git(self) -> Tuple[Set, Set, Set]:
             The following code segment retrieves all relevant untracked files that were changed in the external contribution PR
             and adds them to `modified_files`. See CIAC-10490 for more info.
             """
-            # filter out a string list of untracked files with a path that starts with "Packs/"
-            untracked_files_list = filter(
-                lambda f: f.startswith("Packs/"), self.git_util.repo.untracked_files
-            )
             logger.info(
-                f"\n[cyan]Running on untracked files: {untracked_files_list}[/cyan]"
+                "\n[cyan]CONTRIB_BRANCH variable found, trying to collected changed untracked files from external contribution PR[/cyan]"
             )
-            # convert the string list of untracked files to a set of Path object
-            untracked_files_paths = set(map(Path, untracked_files_list))
-            modified_files = modified_files.union(untracked_files_paths)
+            valid_untracked_files_paths = self.get_untracked_files_in_content()
+            modified_files = modified_files.union(valid_untracked_files_paths)
 
         return modified_files, added_files, renamed_files
 
+    def get_untracked_files_in_content(self) -> Set[Path]:
+        """
+        Filter out a string list of untracked files with a path thats inside the build machine's content repository.
+        The file paths in the build machine are relative so we use absolute path (resolve) to make sure the files are in content.
+        """
+        untracked_files_paths = {
+            Path(f)
+            for f in self.git_util.repo.untracked_files
+            if self.is_relative_to(path=Path(f).resolve(), base=CONTENT_PATH)
+        }
+        logger.info(f"\n######## - Modified untracked:\n{untracked_files_paths}")
+        return untracked_files_paths
+
+    def is_relative_to(self, path: Path, base: Path) -> bool:
+        """
+        The Path class in Python's pathlib module got the native is_relative_to method starting
+        from Python 3.9, so it fails on python 3.8.
+        This is a custom implementation for the is_relative_to method.
+        """
+        try:
+            path.relative_to(base)
+            return True
+        except ValueError:
+            return False
+
     def specify_files_by_status(
         self,
         modified_files: Set,

diff --git a/demisto_sdk/commands/validate/tests/validators_test.py b/demisto_sdk/commands/validate/tests/validators_test.py
@@ -687,11 +687,12 @@ def test_description():
 
 
 @pytest.mark.parametrize(
-    "untracked_files, modified_files, expected_output",
+    "untracked_files, modified_files, untracked_files_in_content ,expected_output",
     [
         (
             ["Packs/untracked.txt"],
-            set({Path("Packs/modified.txt")}),
+            set([Path("Packs/modified.txt")]),
+            set([Path("Packs/untracked.txt")]),
             set([Path("Packs/modified.txt"), Path("Packs/untracked.txt")]),
         ),
         (
@@ -701,7 +702,13 @@ def test_description():
                 "invalid/path/untracked.txt",
                 "another/invalid/path/untracked.txt",
             ],
-            set({Path("Packs/modified.txt")}),
+            set([Path("Packs/modified.txt")]),
+            set(
+                [
+                    Path("Packs/untracked_1.txt"),
+                    Path("Packs/untracked_2.txt"),
+                ]
+            ),
             set(
                 [
                     Path("Packs/modified.txt"),
@@ -724,6 +731,12 @@ def test_description():
                     Path("Packs/untracked_2.txt"),
                 ]
             ),
+            set(
+                [
+                    Path("Packs/untracked_1.txt"),
+                    Path("Packs/untracked_2.txt"),
+                ]
+            ),
         ),
     ],
     ids=[
@@ -733,7 +746,11 @@ def test_description():
     ],
 )
 def test_get_unfiltered_changed_files_from_git_in_external_pr_use_case(
-    mocker, untracked_files, modified_files, expected_output
+    mocker,
+    untracked_files,
+    modified_files,
+    untracked_files_in_content,
+    expected_output,
 ):
     """
     This UT verifies changes made to validate command to support collection of
@@ -761,5 +778,10 @@ def test_get_unfiltered_changed_files_from_git_in_external_pr_use_case(
     mocker.patch(
         "git.repo.base.Repo._get_untracked_files", return_value=untracked_files
     )
+    mocker.patch.object(
+        initializer,
+        "get_untracked_files_in_content",
+        return_value=untracked_files_in_content,
+    )
     output = initializer.get_unfiltered_changed_files_from_git()
     assert output[0] == expected_output