-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat(scm): Add vendor-agnostic interfaces #107360
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
base: master
Are you sure you want to change the base?
Conversation
| organization_id: int, | ||
| integration: Integration | RpcIntegration, | ||
| ) -> Provider: | ||
| client = integration.get_installation(organization_id=organization_id).get_client() |
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.
Instead of using the client directly, would it make sense to add to the existing GitHubIntegration class? While that class is fairly large in implementation, it does include quite a bit of logic that could simplify some of these provider implementations.
The existing class also includes feature-based metric collection and error handling that would need to be reimplemented here as well.
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.
We'll work our way up to it if necessary. Likely we'll just port features from that class as we'll need a generic contract design and callers will need to conform. Leaving existing code in place means the migration path is async.
Add actions and tests (prune unnecessary; add missing):
Each action has its own contract. It needs to return a generic response that effectively communicates what callers want to receive. It should also include the raw response and the provider type so that deeper introspection can be accomplish for service specific customization.
Add provider implementations:
Integration tests for:
Integration tests for: