-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
chore(integrations): register integration domains in initializer #80212
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,11 @@ | ||
from __future__ import annotations | ||
|
||
from collections import defaultdict | ||
from collections.abc import Iterable, Iterator | ||
from typing import Any | ||
|
||
from sentry.exceptions import NotRegistered | ||
from sentry.integrations.base import IntegrationProvider | ||
from sentry.integrations.base import IntegrationDomain, IntegrationProvider | ||
|
||
__all__ = ["IntegrationManager"] | ||
|
||
|
@@ -14,6 +15,12 @@ | |
class IntegrationManager: | ||
def __init__(self) -> None: | ||
self.__values: dict[str, type[IntegrationProvider]] = {} | ||
self.__domain_integrations: dict[IntegrationDomain, list[str]] = defaultdict( | ||
list | ||
) # integrations by domain | ||
self.__integration_domains: dict[str, IntegrationDomain] = ( | ||
{} | ||
) # map of integration to domain | ||
Comment on lines
+21
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor nit: I think it's fine to generate this on the fly from the list of integration providers instead of storing it on the class. I don't think it'll add too much overhead and reduces our need to remove them when something unregisters. |
||
|
||
def __iter__(self) -> Iterator[IntegrationProvider]: | ||
return iter(self.all()) | ||
|
@@ -36,6 +43,9 @@ def exists(self, key: str) -> bool: | |
|
||
def register(self, cls: type[IntegrationProvider]) -> None: | ||
self.__values[cls.key] = cls | ||
if cls.domain: | ||
self.__domain_integrations[cls.domain].append(cls.key) | ||
self.__integration_domains[cls.key] = cls.domain | ||
|
||
def unregister(self, cls: type[IntegrationProvider]) -> None: | ||
try: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you do decide to keep the domain dictionaries around, you'll need to remove integrations from them here. |
||
|
@@ -47,6 +57,19 @@ def unregister(self, cls: type[IntegrationProvider]) -> None: | |
return | ||
del self.__values[cls.key] | ||
|
||
def get_integrations_for_domain(self, domain: IntegrationDomain) -> list[str]: | ||
if domain not in self.__domain_integrations: | ||
return [] | ||
return self.__domain_integrations[domain] | ||
|
||
def get_integrations_by_domain(self) -> dict[IntegrationDomain, list[str]]: | ||
return self.__domain_integrations | ||
|
||
def get_integration_domain(self, key: str) -> IntegrationDomain: | ||
if key not in self.__integration_domains: | ||
raise NotRegistered(key) | ||
return self.__integration_domains[key] | ||
|
||
|
||
default_manager = IntegrationManager() | ||
all = default_manager.all | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely a bit nicer. Tying the domain to the provider class really solidifies what an integration's responsibility is at instantiation time. I also remember @ameliahsu mentioning turning this into an array, which should work for the multiple domains on a single integration problem, but is the issue that we need a single "primary" domain per integration?