Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Integration][Gitlab] - Introduce Pagination and Run Code in Async #1047

Merged
merged 9 commits into from
Sep 25, 2024
Merged

[Integration][Gitlab] - Introduce Pagination and Run Code in Async #1047

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cf750d8
run code in async
PeyGis Sep 24, 2024
a64c561
create changelog
PeyGis Sep 24, 2024
91de5b9
remove page size
PeyGis Sep 24, 2024
e779b70
add pytest
PeyGis Sep 24, 2024
53df017
restructure event processor
PeyGis Sep 24, 2024
95b2c0e
add transient error handling
Tankilevitch Sep 25, 2024
bc7c1b8
add more verbosity
Tankilevitch Sep 25, 2024
8ceed91
lint
Tankilevitch Sep 25, 2024
111a2fd
lint files
PeyGis Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions integrations/gitlab/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

<!-- towncrier release notes start -->

0.1.124 (2024-09-24)
====================

### Improvements

- Added more logs and implemented the webhook creation in async (0.1.124)


0.1.123 (2024-09-22)
====================

Expand Down
13 changes: 5 additions & 8 deletions integrations/gitlab/gitlab_integration/events/event_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,17 +69,14 @@ def on(self, events: list[str], observer: Observer) -> None:
self._observers[event].append(observer)

async def _notify(self, event_id: str, body: dict[str, Any]) -> None:
observers = asyncio.gather(
*(
observer(event_id, body)
for observer in self._observers.get(event_id, [])
)
)

if not observers:
observers_list = self._observers.get(event_id, [])
if not observers_list:
logger.info(
f"event: {event_id} has no matching handler. the handlers available are for events: {self._observers.keys()}"
)
return

await asyncio.gather(*(observer(event_id, body) for observer in observers_list))


class SystemEventHandler(BaseEventHandler):
Expand Down
14 changes: 8 additions & 6 deletions integrations/gitlab/gitlab_integration/events/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ def setup_system_listeners(gitlab_clients: list[GitlabService]) -> None:
system_event_handler.add_client(gitlab_service)


def create_webhooks_by_client(
async def create_webhooks_by_client(
gitlab_host: str,
app_host: str,
token: str,
Expand All @@ -160,7 +160,7 @@ def create_webhooks_by_client(
gitlab_client = Gitlab(gitlab_host, token)
gitlab_service = GitlabService(gitlab_client, app_host, group_mapping)

groups_for_webhooks = gitlab_service.get_filtered_groups_for_webhooks(
groups_for_webhooks = await gitlab_service.get_filtered_groups_for_webhooks(
list(groups_hooks_events_override.keys())
if groups_hooks_events_override
else None
Expand All @@ -169,7 +169,7 @@ def create_webhooks_by_client(
webhooks_ids: list[str] = []

for group in groups_for_webhooks:
webhook_id = gitlab_service.create_webhook(
webhook_id = await gitlab_service.create_webhook(
group,
(
groups_hooks_events_override.get(
Expand All @@ -186,7 +186,7 @@ def create_webhooks_by_client(
return gitlab_service, webhooks_ids


def setup_application(
async def setup_application(
token_mapping: dict[str, list[str]],
gitlab_host: str,
app_host: str,
Expand All @@ -196,13 +196,15 @@ def setup_application(
validate_token_mapping(token_mapping)

if use_system_hook:
logger.info("Using system hook")
validate_use_system_hook(token_mapping)
token, group_mapping = list(token_mapping.items())[0]
gitlab_client = Gitlab(gitlab_host, token)
gitlab_service = GitlabService(gitlab_client, app_host, group_mapping)
setup_system_listeners([gitlab_service])

else:
logger.info("Using group hooks")
validate_hooks_override_config(
token_mapping, token_group_override_hooks_mapping
)
Expand All @@ -212,7 +214,7 @@ def setup_application(
for token, group_mapping in token_mapping.items():
if not token_group_override_hooks_mapping:
client_to_webhooks.append(
create_webhooks_by_client(
await create_webhooks_by_client(
gitlab_host,
app_host,
token,
Expand All @@ -226,7 +228,7 @@ def setup_application(
).groups
if groups:
client_to_webhooks.append(
create_webhooks_by_client(
await create_webhooks_by_client(
gitlab_host,
app_host,
token,
Expand Down
60 changes: 38 additions & 22 deletions integrations/gitlab/gitlab_integration/gitlab_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,22 +63,25 @@ def __init__(
GITLAB_SEARCH_RATE_LIMIT * 0.95, 60
)

def _get_webhook_for_group(self, group: RESTObject) -> RESTObject | None:
async def _get_webhook_for_group(self, group: RESTObject) -> RESTObject | None:
webhook_url = f"{self.app_host}/integration/hook/{group.get_id()}"
for hook in group.hooks.list(iterator=True):
if hook.url == webhook_url:
return hook
async for hook_batch in AsyncFetcher.fetch_batch(
group.hooks.list
):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets create a method in the client for it

for hook in hook_batch:
if hook.url == webhook_url:
return hook
return None

def _delete_group_webhook(self, group: RESTObject, hook_id: int) -> None:
async def _delete_group_webhook(self, group: RESTObject, hook_id: int) -> None:
logger.info(f"Deleting webhook with id {hook_id} in group {group.get_id()}")
try:
group.hooks.delete(hook_id)
await AsyncFetcher.fetch_single(group.hooks.delete, hook_id)
logger.info(f"Deleted webhook for {group.get_id()}")
except Exception as e:
logger.error(f"Failed to delete webhook for {group.get_id()} error={e}")

def _create_group_webhook(
async def _create_group_webhook(
self, group: RESTObject, events: list[str] | None
) -> None:
webhook_events = {
Expand All @@ -90,11 +93,12 @@ def _create_group_webhook(
f"Creating webhook for {group.get_id()} with events: {[event for event in webhook_events if webhook_events[event]]}"
)
try:
resp = group.hooks.create(
resp = await AsyncFetcher.fetch_single(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like maybe fetch_single isn't the appropriate name for it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I felt the same but I was also worried about duplication since the implementation will look the same, only the name of the method will change

group.hooks.create,
{
"url": f"{self.app_host}/integration/hook/{group.get_id()}",
**webhook_events,
}
},
)
logger.info(
f"Created webhook for {group.get_id()}, id={resp.id}, url={resp.url}"
Expand Down Expand Up @@ -253,14 +257,24 @@ def should_process_project(
return True
return project.name in repos

def get_root_groups(self) -> List[Group]:
groups = self.gitlab_client.groups.list(iterator=True)
async def get_root_groups(self) -> List[Group]:
groups = []
async for groups_batch in AsyncFetcher.fetch_batch(
self.gitlab_client.groups.list
):
groups.extend(groups_batch)

return typing.cast(
List[Group], [group for group in groups if group.parent_id is None]
)

def filter_groups_by_paths(self, groups_full_paths: list[str]) -> List[Group]:
groups = self.gitlab_client.groups.list(get_all=True)
async def filter_groups_by_paths(self, groups_full_paths: list[str]) -> List[Group]:
groups = []
async for groups_batch in AsyncFetcher.fetch_batch(
self.gitlab_client.groups.list
):
groups.extend(groups_batch)

return typing.cast(
List[Group],
[
Expand All @@ -270,17 +284,17 @@ def filter_groups_by_paths(self, groups_full_paths: list[str]) -> List[Group]:
],
)

def get_filtered_groups_for_webhooks(
async def get_filtered_groups_for_webhooks(
self,
groups_hooks_override_list: list[str] | None,
) -> List[Group]:
groups_for_webhooks = []
if groups_hooks_override_list is not None:
if groups_hooks_override_list:
logger.info(
"Getting all the specified groups in the mapping for a token to create their webhooks"
f"Getting all the specified groups in the mapping for a token to create their webhooks for: {groups_hooks_override_list}"
)
groups_for_webhooks = self.filter_groups_by_paths(
groups_for_webhooks = await self.filter_groups_by_paths(
groups_hooks_override_list
)

Expand All @@ -302,7 +316,7 @@ def get_filtered_groups_for_webhooks(
)
else:
logger.info("Getting all the root groups to create their webhooks")
root_groups = self.get_root_groups()
root_groups = await self.get_root_groups()
groups_for_webhooks = [
group
for group in root_groups
Expand All @@ -316,7 +330,9 @@ def get_filtered_groups_for_webhooks(

return groups_for_webhooks

def create_webhook(self, group: Group, events: list[str] | None) -> str | None:
async def create_webhook(
self, group: Group, events: list[str] | None
) -> str | None:
logger.info(f"Creating webhook for the group: {group.attributes['full_path']}")

webhook_id = None
Expand All @@ -325,19 +341,19 @@ def create_webhook(self, group: Group, events: list[str] | None) -> str | None:
if group_id is None:
logger.info(f"Group {group.attributes['full_path']} has no id. skipping...")
else:
hook = self._get_webhook_for_group(group)
hook = await self._get_webhook_for_group(group)
if hook:
logger.info(f"Webhook already exists for group {group.get_id()}")

if hook.alert_status == "disabled":
logger.info(
f"Webhook exists for group {group.get_id()} but is disabled, deleting and re-creating..."
)
self._delete_group_webhook(group, hook.id)
self._create_group_webhook(group, events)
await self._delete_group_webhook(group, hook.id)
await self._create_group_webhook(group, events)
logger.info(f"Webhook re-created for group {group.get_id()}")
else:
self._create_group_webhook(group, events)
await self._create_group_webhook(group, events)
webhook_id = str(group_id)

return webhook_id
Expand Down
7 changes: 2 additions & 5 deletions integrations/gitlab/gitlab_integration/ocean.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ async def on_start() -> None:
)

try:
setup_application(
await setup_application(
integration_config["token_mapping"],
integration_config["gitlab_host"],
integration_config["app_host"],
Expand All @@ -102,10 +102,7 @@ async def on_start() -> None:
await event_handler.start_event_processor()
await system_event_handler.start_event_processor()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets run them either way if app_host is configured

except Exception as e:
logger.warning(
f"Failed to setup webhook: {e}. {NO_WEBHOOK_WARNING}",
stack_info=True,
)
logger.exception(f"Failed to setup webhook: {e}. {NO_WEBHOOK_WARNING}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to make sure that it actually prints the stacktrace, I think exception does it but just lets make sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a division by zero with or without the stack_info=True and the log is the same, so it doesn't have any impact

Screenshot 2024-09-24 at 8 07 21 PM



@ocean.on_resync(ObjectKind.GROUP)
Expand Down
2 changes: 1 addition & 1 deletion integrations/gitlab/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[tool.poetry]
name = "gitlab"
version = "0.1.123"
version = "0.1.124"
description = "Gitlab integration for Port using Port-Ocean Framework"
authors = ["Yair Siman-Tov <yair@getport.io>"]

Expand Down
2 changes: 1 addition & 1 deletion integrations/gitlab/tests/gitlab_integration/test_gitlab_service_webhook.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from unittest.mock import MagicMock
from unittest.mock import MagicMock, AsyncMock
from typing import Any
from gitlab_integration.gitlab_service import GitlabService

Expand All @@ -17,7 +17,7 @@
result = mocked_gitlab_service._get_webhook_for_group(mock_group)

# Assert
assert result == mock_hook

Check failure on line 20 in integrations/gitlab/tests/gitlab_integration/test_gitlab_service_webhook.py

View workflow job for this annotation

GitHub Actions / JUnit Test Report

test_gitlab_service_webhook.test_get_webhook_for_group_found 

AssertionError: assert <coroutine object GitlabService._get_webhook_for_group at 0x7f1d3c8616c0> == <MagicMock id='139763546644240'>
Raw output 
mocked_gitlab_service = <gitlab_integration.gitlab_service.GitlabService object at 0x7f1d3d59e610>

    def test_get_webhook_for_group_found(mocked_gitlab_service: GitlabService) -> None:
        # Arrange
        mock_group = MagicMock()
        mock_group.get_id.return_value = 456
        mock_webhook_url = "http://example.com/integration/hook/456"
        mock_hook = MagicMock()
        mock_hook.url = mock_webhook_url
        mock_hook.id = 984
        mock_group.hooks.list.return_value = [mock_hook]
    
        # Act
        result = mocked_gitlab_service._get_webhook_for_group(mock_group)
    
        # Assert
>       assert result == mock_hook
E       AssertionError: assert <coroutine object GitlabService._get_webhook_for_group at 0x7f1d3c8616c0> == <MagicMock id='139763546644240'>

tests/gitlab_integration/test_gitlab_service_webhook.py:20: AssertionError
mock_group.hooks.list.assert_called_once_with(iterator=True)


Expand All @@ -33,7 +33,7 @@
result = mocked_gitlab_service._get_webhook_for_group(mock_group)

# Assert
assert result is None

Check failure on line 36 in integrations/gitlab/tests/gitlab_integration/test_gitlab_service_webhook.py

View workflow job for this annotation

GitHub Actions / JUnit Test Report

test_gitlab_service_webhook.test_get_webhook_for_group_not_found 

assert <coroutine object GitlabService._get_webhook_for_group at 0x7f604d894310> is None
Raw output 
mocked_gitlab_service = <gitlab_integration.gitlab_service.GitlabService object at 0x7f6048faa7d0>

    def test_get_webhook_for_group_not_found(mocked_gitlab_service: GitlabService) -> None:
        # Arrange
        mock_group = MagicMock()
        mock_group.get_id.return_value = 789
        mock_hook = MagicMock()
        mock_hook.url = "http://example.com/other/hook"
        mock_group.hooks.list.return_value = [mock_hook]
    
        # Act
        result = mocked_gitlab_service._get_webhook_for_group(mock_group)
    
        # Assert
>       assert result is None
E       assert <coroutine object GitlabService._get_webhook_for_group at 0x7f604d894310> is None

tests/gitlab_integration/test_gitlab_service_webhook.py:36: AssertionError
mock_group.hooks.list.assert_called_once_with(iterator=True)


Expand Down Expand Up @@ -68,7 +68,7 @@
)

# Assert
assert webhook_id == "456"

Check failure on line 71 in integrations/gitlab/tests/gitlab_integration/test_gitlab_service_webhook.py

View workflow job for this annotation

GitHub Actions / JUnit Test Report

test_gitlab_service_webhook.test_create_webhook_when_webhook_exists_but_disabled 

AssertionError: assert <coroutine object GitlabService.create_webhook at 0x7f1d3c878f40> == '456'
Raw output 
mocked_gitlab_service = <gitlab_integration.gitlab_service.GitlabService object at 0x7f1d3c85f4d0>
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7f1d3c61d690>

    def test_create_webhook_when_webhook_exists_but_disabled(
        mocked_gitlab_service: GitlabService, monkeypatch: Any
    ):
        # Arrange
        mock_group = MagicMock()
        mock_group.get_id.return_value = 456
        mock_group.attributes = {"full_path": "group2"}
    
        # Mock the group hooks.list method to return an existing disabled webhook
        mock_hook = MagicMock()
        mock_hook.url = "http://example.com/integration/hook/456"  # Updated URL for clarity
        mock_hook.alert_status = "disabled"
        mock_hook.id = 456
        mock_group.hooks.list.return_value = [mock_hook]
    
        # Mock the methods for deleting and creating webhooks
        mock_delete_webhook = MagicMock()
        monkeypatch.setattr(
            mocked_gitlab_service, "_delete_group_webhook", mock_delete_webhook
        )
        mock_create_webhook = MagicMock()
        monkeypatch.setattr(
            mocked_gitlab_service, "_create_group_webhook", mock_create_webhook
        )
    
        # Act
        webhook_id = mocked_gitlab_service.create_webhook(
            mock_group, events=["push", "merge_request"]
        )
    
        # Assert
>       assert webhook_id == "456"
E       AssertionError: assert <coroutine object GitlabService.create_webhook at 0x7f1d3c878f40> == '456'

tests/gitlab_integration/test_gitlab_service_webhook.py:71: AssertionError
mock_delete_webhook.assert_called_once_with(
mock_group, mock_hook.id
) # Ensure delete method is called
Expand Down Expand Up @@ -104,7 +104,7 @@
)

# Assert
assert webhook_id == "789"

Check failure on line 107 in integrations/gitlab/tests/gitlab_integration/test_gitlab_service_webhook.py

View workflow job for this annotation

GitHub Actions / JUnit Test Report

test_gitlab_service_webhook.test_create_webhook_when_webhook_exists_and_enabled 

AssertionError: assert <coroutine object GitlabService.create_webhook at 0x7fc11fc6f140> == '789'
Raw output 
mocked_gitlab_service = <gitlab_integration.gitlab_service.GitlabService object at 0x7fc11e716ed0>
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7fc11e6e1010>

    def test_create_webhook_when_webhook_exists_and_enabled(
        mocked_gitlab_service: GitlabService, monkeypatch: Any
    ):
        # Arrange
        mock_group = MagicMock()
        mock_group.get_id.return_value = 789
        mock_group.attributes = {"full_path": "group3"}
    
        # Mock the group hooks.list method to return an existing enabled webhook
        mock_hook = MagicMock()
        mock_hook.url = "http://example.com/integration/hook/789"
        mock_hook.alert_status = "executable"
        mock_hook.id = 789
        mock_group.hooks.list.return_value = [mock_hook]
    
        # Mock the method for creating webhooks
        mock_create_webhook = MagicMock()
        monkeypatch.setattr(
            mocked_gitlab_service, "_create_group_webhook", mock_create_webhook
        )
    
        # Act
        webhook_id = mocked_gitlab_service.create_webhook(
            mock_group, events=["push", "merge_request"]
        )
    
        # Assert
>       assert webhook_id == "789"
E       AssertionError: assert <coroutine object GitlabService.create_webhook at 0x7fc11fc6f140> == '789'

tests/gitlab_integration/test_gitlab_service_webhook.py:107: AssertionError
mock_create_webhook.assert_not_called() # Ensure no new webhook is created


Expand All @@ -125,7 +125,7 @@
)

# Assert
assert webhook_id == "123"

Check failure on line 128 in integrations/gitlab/tests/gitlab_integration/test_gitlab_service_webhook.py

View workflow job for this annotation

GitHub Actions / JUnit Test Report

test_gitlab_service_webhook.test_create_webhook_when_no_webhook_exists 

AssertionError: assert <coroutine object GitlabService.create_webhook at 0x7effecb2f540> == '123'
Raw output 
mocked_gitlab_service = <gitlab_integration.gitlab_service.GitlabService object at 0x7effecbc4550>
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7effecba2f50>

    def test_create_webhook_when_no_webhook_exists(
        mocked_gitlab_service: GitlabService, monkeypatch: Any
    ):
        # Arrange
        mock_group = MagicMock()
        mock_group.get_id.return_value = 123
        mock_group.attributes = {"full_path": "group1"}
    
        # Mock the group hooks.list method to return no webhook
        mock_group.hooks.list.return_value = []
    
        # Act
        webhook_id = mocked_gitlab_service.create_webhook(
            mock_group, events=["push", "merge_request"]
        )
    
        # Assert
>       assert webhook_id == "123"
E       AssertionError: assert <coroutine object GitlabService.create_webhook at 0x7effecb2f540> == '123'

tests/gitlab_integration/test_gitlab_service_webhook.py:128: AssertionError
mock_group.hooks.create.assert_called_once() # A new webhook should be created


Expand All @@ -145,6 +145,6 @@
mocked_gitlab_service._delete_group_webhook(mock_group, mock_hook.id)

# Assert
mock_group.hooks.delete.assert_called_once_with(

Check failure on line 148 in integrations/gitlab/tests/gitlab_integration/test_gitlab_service_webhook.py

View workflow job for this annotation

GitHub Actions / JUnit Test Report

test_gitlab_service_webhook.test_delete_webhook 

AssertionError: Expected 'delete' to be called once. Called 0 times.
Raw output 
mocked_gitlab_service = <gitlab_integration.gitlab_service.GitlabService object at 0x7f6048fb27d0>
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7f6048d79c90>

    def test_delete_webhook(mocked_gitlab_service: GitlabService, monkeypatch: Any):
        # Arrange
        mock_group = MagicMock()
        mock_group.get_id.return_value = 456
        mock_group.attributes = {"full_path": "group2"}
    
        # Mock the group hooks.list method to return a webhook
        mock_hook = MagicMock()
        mock_hook.url = "http://example.com/integration/hook/456"
        mock_hook.id = 17
        mock_group.hooks.list.return_value = [mock_hook]
    
        # Act
        mocked_gitlab_service._delete_group_webhook(mock_group, mock_hook.id)
    
        # Assert
>       mock_group.hooks.delete.assert_called_once_with(
            mock_hook.id
        )  # Ensure the webhook is deleted

tests/gitlab_integration/test_gitlab_service_webhook.py:148: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <MagicMock name='mock.hooks.delete' id='140051515723792'>, args = (17,)
kwargs = {}, msg = "Expected 'delete' to be called once. Called 0 times."

    def assert_called_once_with(self, /, *args, **kwargs):
        """assert that the mock was called exactly once and that that call was
        with the specified arguments."""
        if not self.call_count == 1:
            msg = ("Expected '%s' to be called once. Called %s times.%s"
                   % (self._mock_name or 'mock',
                      self.call_count,
                      self._calls_repr()))
>           raise AssertionError(msg)
E           AssertionError: Expected 'delete' to be called once. Called 0 times.

/opt/hostedtoolcache/Python/3.11.10/x64/lib/python3.11/unittest/mock.py:950: AssertionError
mock_hook.id
) # Ensure the webhook is deleted
Loading