From 82ef835c154d8effe29190344a700d993068776a Mon Sep 17 00:00:00 2001 From: TigreModerata Date: Mon, 26 Feb 2024 18:21:55 +0100 Subject: [PATCH] ReviewIssue685 --- .../processors/project/hooks_processor.py | 60 +++++---- tests/acceptance/standard/test_hooks.py | 122 ++++++++++++++---- 2 files changed, 129 insertions(+), 53 deletions(-) diff --git a/gitlabform/processors/project/hooks_processor.py b/gitlabform/processors/project/hooks_processor.py index b448e0d2b..c1385e98f 100644 --- a/gitlabform/processors/project/hooks_processor.py +++ b/gitlabform/processors/project/hooks_processor.py @@ -15,52 +15,60 @@ def __init__(self, gitlab: GitLab): 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_id = hook_in_gitlab.id if hook_in_gitlab else None if configuration.get("hooks|" + hook + "|delete"): if hook_id: - debug("Deleting hook '%s'", hook) + debug(f"Deleting hook '{hook}'") project.hooks.delete(hook_id) else: - debug("Not deleting hook '%s', because it doesn't exist", hook) + debug(f"Not deleting hook '{hook}', because it doesn't exist") else: hook_config = {"url": hook} hook_config.update(configuration["hooks"][hook]) - gl_hook_dict = gitlab_hook.asdict() if gitlab_hook else {} + if not hook_id: + debug(f"Creating hook '{hook}'") + created_hook: RESTObject = project.hooks.create(hook_config) + debug(f"Created hook: {created_hook}") + continue + + gl_hook: dict = hook_in_gitlab.asdict() if hook_in_gitlab else {} + if "token" in hook_config: + debug( + f"The hook '{hook}' config includes a token. Diff between config vs gitlab cannot be confirmed" + ) + gl_hook["token"] = "" + diffs = ( map( - lambda k: hook_config[k] != gl_hook_dict[k], + lambda k: hook_config[k] != gl_hook[k], hook_config.keys(), ) - if gl_hook_dict + if gl_hook else iter(()) ) - 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 + if any(diffs): + debug( + f"The hook '{hook}' config is different from what's in gitlab OR it contains a token" ) - 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") 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) diff --git a/tests/acceptance/standard/test_hooks.py b/tests/acceptance/standard/test_hooks.py index 8182f5cb2..33c16e25a 100644 --- a/tests/acceptance/standard/test_hooks.py +++ b/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,79 @@ 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 + # 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): 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) delete_yaml = f""" projects_and_groups: @@ -101,23 +144,36 @@ 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 """ 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() 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 +187,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 +206,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 +231,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 +249,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