-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
chore(scm): metrics for derive_code_mappings #81329
Conversation
) | ||
|
||
def test_does_not_raise_installation_removed(self): | ||
def test_does_not_raise_installation_removed(self, mock_record): |
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 test wasn't testing anything because the platform "any" was not in the list of SUPPORTED_LANGUAGES
. fixed that above
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.
lgtm, 1 nit
try: | ||
with lock.acquire(): | ||
# This method is specific to the GithubIntegration | ||
if not isinstance(installation, GitHubIntegration): |
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.
noice
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.
LGTM, minor comments but nothing blocking.
return self.respond( | ||
{ | ||
"text": f"The {installation.model.provider} integration does not support derived code mappings" | ||
}, | ||
status=status.HTTP_400_BAD_REQUEST, |
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.
Should we ever hit this case? If so, do we want to log here?
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.
no, the query for the installation only filters for github integrations. this fixes the typing but i need to return early and return something
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #81329 +/- ##
===========================================
+ Coverage 52.90% 80.35% +27.45%
===========================================
Files 7195 7220 +25
Lines 318237 319588 +1351
Branches 20772 20772
===========================================
+ Hits 168369 256814 +88445
+ Misses 149475 62381 -87094
Partials 393 393 |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Adds SLO tracking for the
derive_code_mappings
task, which is currently only used for GitHub integrations.