chore(integrations): register integration domains in initializer #80212
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #80212 +/- ##
==========================================
- Coverage 78.08% 78.08% -0.01%
==========================================
Files 7179 7179
Lines 317130 317148 +18
Branches 43724 43727 +3
==========================================
+ Hits 247637 247639 +2
- Misses 63148 63158 +10
- Partials 6345 6351 +6 |
| @@ -311,6 +311,7 @@ def search_issues(self, query: str | None, **kwargs) -> dict[str, Any]: | |||
| class GitHubIntegrationProvider(IntegrationProvider): | |||
| key = "github" | |||
| name = "GitHub" | |||
| domain = IntegrationDomain.SOURCE_CODE_MANAGEMENT | |||
There was a problem hiding this comment.
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?
| self.__integration_domains: dict[str, IntegrationDomain] = ( | ||
| {} | ||
| ) # map of integration to domain |
There was a problem hiding this comment.
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.
| @@ -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.
If you do decide to keep the domain dictionaries around, you'll need to remove integrations from them here.
Instead of hardcoding the integration domains and their keys, we can specify the
domaininside each respectiveIntegrationProvider. When the provider gets registered ininitializer.py, we can dynamically construct the mapping of domains to keys, and vice versa.domainis optional as there are some integrations that we haven't classified yet. If/when we emit metrics, we can classify these under "unknown".