fix: hooks processing fails when config includes token

gitlabform/processors/project/hooks_processor.py

@@ -15,52 +15,56 @@
     def _process_configuration(self, project_and_group: str, configuration: dict):
         debug("Processing hooks...")
         project: Project = self.gl.projects.get(project_and_group)
-        hooks_list: RESTObjectList | List[RESTObject] = project.hooks.list()
-        config_hooks: tuple[str, ...] = tuple(
+        project_hooks: RESTObjectList | List[RESTObject] = project.hooks.list()
+        hooks_in_config: tuple[str, ...] = tuple(
             x for x in sorted(configuration["hooks"]) if x != "enforce"
         )
 
-        for hook in config_hooks:
-            gitlab_hook: RESTObject | None = next(
-                (h for h in hooks_list if h.url == hook), None
+        for hook in hooks_in_config:
+            hook_in_gitlab: RESTObject | None = next(
+                (h for h in project_hooks if h.url == hook), None
             )
-            hook_id = gitlab_hook.id if gitlab_hook else None
+            hook_config = {"url": hook}
+            hook_config.update(configuration["hooks"][hook])
+
+            hook_id = hook_in_gitlab.id if hook_in_gitlab else None
+
+            # Process hooks configured for deletion
             if configuration.get("hooks|" + hook + "|delete"):
                 if hook_id:
-                    debug("Deleting hook '%s'", hook)
+                    debug(f"Deleting hook '{hook}'")
                     project.hooks.delete(hook_id)
+                    debug(f"Deleted hook '{hook}'")
                 else:
-                    debug("Not deleting hook '%s', because it doesn't exist", hook)
-            else:
-                hook_config = {"url": hook}
-                hook_config.update(configuration["hooks"][hook])
-                gl_hook_dict = gitlab_hook.asdict() if gitlab_hook else {}
-                diffs = (
-                    map(
-                        lambda k: hook_config[k] != gl_hook_dict[k],
-                        hook_config.keys(),
-                    )
-                    if gl_hook_dict
-                    else iter(())
+                    debug(f"Not deleting hook '{hook}', because it doesn't exist")
+                continue
+
+            # Process new hook creation
+            if not hook_id:
+                debug(f"Creating hook '{hook}'")
+                created_hook: RESTObject = project.hooks.create(hook_config)
+                debug(f"Created hook: {created_hook}")
+                continue
+
+            # Processing existing hook updates
+            gl_hook: dict = hook_in_gitlab.asdict() if hook_in_gitlab else {}
+            if self._needs_update(gl_hook, hook_config):
+                debug(
+                    f"The hook '{hook}' config is different from what's in gitlab OR it contains a token"
                 )
-                if not hook_id:
-                    debug("Creating hook '%s'", hook)
-                    created_hook: RESTObject = project.hooks.create(hook_config)
-                    debug("Created hook '%s'", created_hook)
-                elif hook_id and any(diffs):
-                    debug("Changing existing hook '%s'", hook)
-                    changed_hook: Dict[str, Any] = project.hooks.update(
-                        hook_id, hook_config
-                    )
-                    debug("Changed hook to '%s'", changed_hook)
-                elif hook_id and not any(diffs):
-                    debug(f"Hook {hook} remains unchanged")
+                debug(f"Updating hook '{hook}'")
+                updated_hook: Dict[str, Any] = project.hooks.update(
+                    hook_id, hook_config
+                )
+                debug(f"Updated hook: {updated_hook}")
+            else:
+                debug(f"Hook '{hook}' remains unchanged")
 
+        # Process hook config enforcements
         if configuration.get("hooks|enforce"):
-            for gh in hooks_list:
-                if gh.url not in config_hooks:
+            for gh in project_hooks:
+                if gh.url not in hooks_in_config:
                     debug(
-                        "Deleting hook '%s' currently setup in the project but it is not in the configuration and enforce is enabled",
-                        gh.url,
+                        f"Deleting hook '{gh.url}' currently setup in the project but it is not in the configuration and enforce is enabled"
                     )
                     project.hooks.delete(gh.id)

tests/acceptance/standard/test_hooks.py

@@ -1,20 +1,20 @@
 import logging
 import pytest
 from typing import TYPE_CHECKING
+from gitlab.v4.objects import ProjectHook
 
 from tests.acceptance import run_gitlabform, get_random_name
 
 
-LOGGER = logging.getLogger(__name__)
-
-
 @pytest.fixture(scope="class")
 def urls():
     first_name = get_random_name("hook")
     second_name = get_random_name("hook")
+    third_name = get_random_name("hook")
     first_url = f"http://hooks/{first_name}.org"
     second_url = f"http://hooks/{second_name}.org"
-    return first_url, second_url
+    third_url = f"http://hooks/{third_name}.com"
+    return first_url, second_url, third_url
 
 
 class TestHooksProcessor:
@@ -23,28 +23,36 @@ def get_hook_from_url(self, project, url):
 
     def test_hooks_create(self, gl, project, urls):
         target = project.path_with_namespace
-        first_url, second_url = urls
+        first_url, second_url, third_url = urls
 
         test_yaml = f"""
             projects_and_groups:
               {target}:
                 hooks:
                   {first_url}:
+                    token: a1b2c3d4
                     push_events: false
                     merge_requests_events: true
                   {second_url}:
                     job_events: true
                     note_events: true
+                  {third_url}:
+                    token: abc123def
+                    push_events: true
+                    merge_requests_events: true
             """
 
         run_gitlabform(test_yaml, target)
 
         first_created_hook = self.get_hook_from_url(project, first_url)
         second_created_hook = self.get_hook_from_url(project, second_url)
+        third_created_hook = self.get_hook_from_url(project, third_url)
+
         if TYPE_CHECKING:
             assert isinstance(first_created_hook, ProjectHook)
             assert isinstance(second_created_hook, ProjectHook)
-        assert len(project.hooks.list()) == 2
+            assert isinstance(third_created_hook, ProjectHook)
+        assert len(project.hooks.list()) == 3
         assert (
             first_created_hook.push_events,
             first_created_hook.merge_requests_events,
@@ -53,44 +61,80 @@ def test_hooks_create(self, gl, project, urls):
             True,
             True,
         )
+        assert (
+            third_created_hook.push_events,
+            third_created_hook.merge_requests_events,
+        ) == (True, True)
 
     def test_hooks_update(self, caplog, gl, project, urls):
-        first_url, second_url = urls
+        first_url, second_url, third_url = urls
         target = project.path_with_namespace
         first_hook = self.get_hook_from_url(project, first_url)
         second_hook = self.get_hook_from_url(project, second_url)
+        third_hook = self.get_hook_from_url(project, third_url)
 
         update_yaml = f"""
             projects_and_groups:
               {target}:
                 hooks:
                   {first_url}:
-                    id: {first_hook.id}
+                    token: a1b2c3d4
                     merge_requests_events: false
                     note_events: true
                   {second_url}:
                     job_events: true
                     note_events: true
+                  {third_url}:
+                    push_events: true
+                    merge_requests_events: true
             """
 
         run_gitlabform(update_yaml, target)
         updated_first_hook = self.get_hook_from_url(project, first_url)
         updated_second_hook = self.get_hook_from_url(project, second_url)
+        updated_third_hook = self.get_hook_from_url(project, third_url)
 
         with caplog.at_level(logging.DEBUG):
-            assert f"Hook {second_url} remains unchanged" in caplog.text
-            assert f"Changing existing hook '{first_url}'" in caplog.text
-        assert updated_first_hook.asdict() != first_hook.asdict()
-        assert updated_second_hook.asdict() == second_hook.asdict()
-        # push events defaults to True, but it stays False
-        assert updated_first_hook.push_events == False
-        assert updated_first_hook.merge_requests_events == False
-        assert updated_first_hook.note_events == True
-
-    def test_hooks_delete(self, gl, project, urls):
+            # The first should be updated and be different than initial config done in previous test case.
+            # The hook contains a token, which is a secret. So, cannot confirm whether it's different from
+            # existing config in. This is why the hook is always updated. The hook's current config is also
+            # different from when it was created in previous test case.
+            assert f"Updating hook '{first_url}'" in caplog.text
+            assert updated_first_hook.asdict() != first_hook.asdict()
+            # push_events stays False from previous test case config
+            assert (
+                updated_first_hook.push_events,
+                updated_first_hook.merge_requests_events,
+                updated_first_hook.note_events,
+            ) == (False, False, True)
+
+            # The second hook should remain unchanged.
+            # The hook did not change from the previous test case. So, updating it is not necessary.
+            assert f"Hook '{second_url}' remains unchanged" in caplog.text
+            assert updated_second_hook.asdict() == second_hook.asdict()
+            assert (
+                updated_second_hook.job_events,
+                updated_second_hook.note_events,
+            ) == (True, True)
+
+            # The third hook should remain unchanged.
+            # The hook initially had a token when it was created in previous test case.
+            # In the current run/config the token is removed but all other configs remain same.
+            # GitLabForm does not have memory or awareness of previous configs. So, comparing with
+            # existing config in GitLab, the hook did not change and is not updated.
+            assert f"Hook '{third_url}' remains unchanged" in caplog.text
+            assert updated_third_hook.asdict() == third_hook.asdict()
+            assert (
+                updated_third_hook.push_events,
+                updated_third_hook.merge_requests_events,
+            ) == (True, True)
+
+    def test_hooks_delete(self, gl, project, urls, caplog):
         target = project.path_with_namespace
-        first_url, second_url = urls
-        orig_second_hook = self.get_hook_from_url(project, second_url)
+        first_url, second_url, third_url = urls
+        second_hook_before_test = self.get_hook_from_url(project, second_url)
+        third_hook_before_test = self.get_hook_from_url(project, third_url)
+        non_existent_hook_url = f"https://unknown_{get_random_name('hook')}.com"
 
         delete_yaml = f"""
         projects_and_groups:
@@ -101,23 +145,45 @@ def test_hooks_delete(self, gl, project, urls):
               {second_url}:
                 job_events: false
                 note_events: false
+              {third_url}:
+                token: abc123def
+                push_events: true
+                merge_requests_events: true
+              {non_existent_hook_url}:
+                delete: true
         """
 
         run_gitlabform(delete_yaml, target)
-        hooks = project.hooks.list()
-        second_hook = self.get_hook_from_url(project, second_url)
+        hooks_after_test = project.hooks.list()
+        second_hook_after_test = self.get_hook_from_url(project, second_url)
+        third_hook_after_test = self.get_hook_from_url(project, third_url)
 
-        assert len(hooks) == 1
-        assert first_url not in (h.url for h in hooks)
-        assert second_hook in hooks
-        assert second_hook == orig_second_hook
+        assert len(hooks_after_test) == 2
+        # The first hook should not exist as indicated by 'delete: true' config
+        assert first_url not in (h.url for h in hooks_after_test)
+        # The second hook should exist but updated as the config is different from
+        # the setup in previous test case.
+        assert second_hook_after_test in hooks_after_test
+        assert second_hook_after_test.asdict() != second_hook_before_test.asdict()
+        # The thrid hook should exist and same as it was setup in previous test case.
+        assert third_hook_after_test in hooks_after_test
+        assert third_hook_after_test.asdict() == third_hook_before_test.asdict()
+        # The last hook configured for deletion but it was never setup in gitlab.
+        # Ensure expected error message is reported.
+        with caplog.at_level(logging.DEBUG):
+            assert (
+                f"Not deleting hook '{non_existent_hook_url}', because it doesn't exist"
+                in caplog.text
+            )
 
     def test_hooks_enforce(self, gl, group, project, urls):
         target = project.path_with_namespace
-        first_url, second_url = urls
+        first_url, second_url, third_url = urls
         hooks_before_test = [h.url for h in project.hooks.list()]
-        assert len(hooks_before_test) == 1
-        assert second_url == hooks_before_test[0]
+
+        # Total number of hooks before the test should match the remaining
+        # hooks at the end of previous test case.
+        assert len(hooks_before_test) == 2
 
         enforce_yaml = f"""
                 projects_and_groups:
@@ -131,9 +197,12 @@ def test_hooks_enforce(self, gl, group, project, urls):
 
         run_gitlabform(enforce_yaml, target)
         hooks_after_test = [h.url for h in project.hooks.list()]
+        # Because of 'enforce: true' config, total number of hooks should be
+        # what's in the applied config.
         assert len(hooks_after_test) == 1
         assert first_url in hooks_after_test
         assert second_url not in hooks_after_test
+        assert third_url not in hooks_after_test
 
         not_enforce_yaml = f"""
                 projects_and_groups:
@@ -147,6 +216,8 @@ def test_hooks_enforce(self, gl, group, project, urls):
 
         run_gitlabform(not_enforce_yaml, target)
         hooks_after_test = [h.url for h in project.hooks.list()]
+        # Because of 'enforce: false', default config, total number of hooks should be
+        # what's in the applied config and what was previously configured.
         assert len(hooks_after_test) == 2
         assert (
             first_url in hooks_after_test
@@ -170,6 +241,9 @@ def test_hooks_enforce(self, gl, group, project, urls):
         run_gitlabform(enforce_star_yaml, target)
         hooks_after_test = [h.url for h in project.hooks.list()]
 
+        # Because 'enforce: true' config is in parent group, it will apply to all projects within the group.
+        # So, the project being tested will contain only the hooks that are applied by the project and also
+        # by the parent group config.
         assert len(hooks_after_test) == 2
         assert first_url in hooks_after_test and second_url in hooks_after_test
         assert "http://www.newhook.org" not in hooks_after_test
@@ -185,4 +259,8 @@ def test_hooks_enforce(self, gl, group, project, urls):
 
         run_gitlabform(enforce_delete_yaml, target)
         hooks_after_test = [h.url for h in project.hooks.list()]
+
+        # The 'enforce: true' config is set, which means only the hooks that are in the config
+        # applied to the project, should exist. But, the only hook in the config is set to be
+        # deleted. So, there should be no hooks remaining.
         assert len(hooks_after_test) == 0