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

feat(commit-context): Do not create if older than 1 year old #54866

Merged
merged 8 commits into from
Aug 17, 2023
Merged

feat(commit-context): Do not create if older than 1 year old #54866

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4fec5b2
feat(commit-context): Do not create if older than 1 year
NisanthanNanthakumar Aug 11, 2023
b366a98
ref date comparison into a separate function
NisanthanNanthakumar Aug 15, 2023
53a1f44
add dates to test fixture
NisanthanNanthakumar Aug 15, 2023
704f922
return datetime objects to make comparisons easy
NisanthanNanthakumar Aug 16, 2023
2e4e0fc
update test fixtures
NisanthanNanthakumar Aug 16, 2023
b442023
add comment
NisanthanNanthakumar Aug 16, 2023
ab340d7
use isodate.parse_datetime
NisanthanNanthakumar Aug 16, 2023
81b93a7
update test
NisanthanNanthakumar Aug 16, 2023
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
11 changes: 8 additions & 3 deletions src/sentry/integrations/github/integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import logging
import re
from datetime import datetime
from datetime import datetime, timezone
from typing import Any, Collection, Dict, Mapping, Sequence

from django.http import HttpResponse
Expand Down Expand Up @@ -242,6 +242,7 @@ def get_commit_context(
except ApiError as e:
raise e

date_format_expected = "%Y-%m-%dT%H:%M:%SZ"
try:
commit: Mapping[str, Any] = max(
(
Expand All @@ -250,7 +251,7 @@ def get_commit_context(
if blame.get("startingLine", 0) <= lineno <= blame.get("endingLine", 0)
),
key=lambda blame: datetime.strptime(
blame.get("commit", {}).get("committedDate"), "%Y-%m-%dT%H:%M:%SZ"
blame.get("commit", {}).get("committedDate"), date_format_expected
),
default={},
)
Expand All @@ -263,9 +264,13 @@ def get_commit_context(
if not commitInfo:
return None
else:
committed_date = datetime.strptime(
commitInfo.get("committed_date"), date_format_expected
).astimezone(timezone.utc)

return {
"commitId": commitInfo.get("oid"),
"committedDate": commitInfo.get("committedDate"),
"committedDate": committed_date,
"commitMessage": commitInfo.get("message"),
"commitAuthorName": commitInfo.get("author", {}).get("name"),
"commitAuthorEmail": commitInfo.get("author", {}).get("email"),
Expand Down
9 changes: 4 additions & 5 deletions src/sentry/integrations/gitlab/integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,11 +181,10 @@ def get_commit_context(
if not commitInfo:
return None
else:
committed_date = "{}Z".format(
datetime.strptime(commitInfo.get("committed_date"), date_format_expected)
.astimezone(timezone.utc)
.strftime("%Y-%m-%dT%H:%M:%S.%f")[:-3]
)
committed_date = datetime.strptime(
commitInfo.get("committed_date"), date_format_expected
).astimezone(timezone.utc)
Copy link
Member

Choose a reason for hiding this comment

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

this is sad that we have to define the format for 8601. Can we put a comment to replace this when we get to python 3.11 and can use dateutil.parser.isoparse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the comment!

Copy link
Member

Choose a reason for hiding this comment

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

there is isodate.parse_date but we don't seem to use it anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm can you link to it?

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/gweis/isodate/#documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored to use isodate.parse_datetime


return {
"commitId": commitInfo.get("id"),
"committedDate": committed_date,
Expand Down
8 changes: 7 additions & 1 deletion src/sentry/integrations/utils/commit_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from __future__ import annotations

import logging
from datetime import datetime, timedelta, timezone
from typing import Any, List, Mapping, Sequence, Tuple

import sentry_sdk
Expand Down Expand Up @@ -108,7 +109,12 @@ def find_commit_context_for_event(
},
)

if commit_context:
# Only return suspect commits that are less than a year old
if commit_context and is_date_less_than_year(commit_context["committedDate"]):
result.append((commit_context, code_mapping))

return result, installation


def is_date_less_than_year(date: datetime):
return date > datetime.now(tz=timezone.utc) - timedelta(days=365)
69 changes: 61 additions & 8 deletions tests/sentry/tasks/test_commit_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from datetime import timedelta
from datetime import datetime, timedelta
from datetime import timezone as datetime_timezone
from unittest.mock import Mock, patch

import pytest
Expand Down Expand Up @@ -83,7 +84,9 @@
"sentry.integrations.github.GitHubIntegration.get_commit_context",
return_value={
"commitId": "asdfwreqr",
"committedDate": "2023-02-14T11:11Z",
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)).strftime(
"%Y-%m-%dT%H:%M:%SZ"
),
"commitMessage": "placeholder commit message",
"commitAuthorName": "",
"commitAuthorEmail": "admin@localhost",
Expand Down Expand Up @@ -114,7 +117,7 @@
type=GroupOwnerType.SUSPECT_COMMIT.value,
).context == {"commitId": self.commit.id}

@patch("sentry.analytics.record")

Check failure on line 120 in tests/sentry/tasks/test_commit_context.py

View workflow job for this annotation

GitHub Actions / backend test (0, 14) 

TestCommitContext__InRegionMode.test_commit_author_no_user

TypeError: '>' not supported between instances of 'str' and 'datetime.datetime'

Check failure on line 120 in tests/sentry/tasks/test_commit_context.py

View workflow job for this annotation

GitHub Actions / backend test (0, 14) 

TestCommitContext__InRegionMode.test_commit_author_not_in_sentry

TypeError: '>' not supported between instances of 'str' and 'datetime.datetime'

Check failure on line 120 in tests/sentry/tasks/test_commit_context.py

View workflow job for this annotation

GitHub Actions / backend test (0, 14) 

TestCommitContext__InRegionMode.test_delete_old_entries

TypeError: '>' not supported between instances of 'str' and 'datetime.datetime'

Check failure on line 120 in tests/sentry/tasks/test_commit_context.py

View workflow job for this annotation

GitHub Actions / backend test (0, 14) 

TestCommitContext__InRegionMode.test_found_commit_is_too_old

TypeError: '>' not supported between instances of 'str' and 'datetime.datetime'

Check failure on line 120 in tests/sentry/tasks/test_commit_context.py

View workflow job for this annotation

GitHub Actions / backend test (0, 14) 

TestCommitContext__InRegionMode.test_multiple_matching_code_mappings_but_only_1_repository_has_the_commit_in_db

TypeError: '>' not supported between instances of 'str' and 'datetime.datetime'

Check failure on line 120 in tests/sentry/tasks/test_commit_context.py

View workflow job for this annotation

GitHub Actions / backend test (0, 14) 

TestCommitContext__InRegionMode.test_no_matching_commit_in_db

TypeError: '>' not supported between instances of 'str' and 'datetime.datetime'

Check failure on line 120 in tests/sentry/tasks/test_commit_context.py

View workflow job for this annotation

GitHub Actions / backend test (0, 14) 

TestCommitContext__InRegionMode.test_simple

TypeError: '>' not supported between instances of 'str' and 'datetime.datetime'

Check failure on line 120 in tests/sentry/tasks/test_commit_context.py

View workflow job for this annotation

GitHub Actions / backend test (0, 14) 

TestCommitContext.test_commit_author_no_user

TypeError: '>' not supported between instances of 'str' and 'datetime.datetime'
@patch(
"sentry.integrations.github.GitHubIntegration.get_commit_context",
side_effect=ApiError(text="integration_failed"),
Expand All @@ -141,11 +144,51 @@
error_message="integration_failed",
)

@patch("sentry.tasks.commit_context.logger")
@patch(
"sentry.integrations.github.GitHubIntegration.get_commit_context",
return_value={
"commitId": "asdfasdf",
"committedDate": "2023-02-14T11:11Z",
"committedDate": (
datetime.now(tz=datetime_timezone.utc) - timedelta(days=370)
).strftime("%Y-%m-%dT%H:%M:%SZ"),
"commitMessage": "placeholder commit message",
"commitAuthorName": "",
"commitAuthorEmail": "admin@localhost",
},
)
def test_found_commit_is_too_old(self, mock_get_commit_context, mock_logger):
with self.tasks():
assert not GroupOwner.objects.filter(group=self.event.group).exists()
event_frames = get_frame_paths(self.event)
process_commit_context(
event_id=self.event.event_id,
event_platform=self.event.platform,
event_frames=event_frames,
group_id=self.event.group_id,
project_id=self.event.project_id,
)

assert mock_logger.info.call_count == 1
mock_logger.info.assert_called_with(
"process_commit_context.find_commit_context",
extra={
"event": self.event.event_id,
"group": self.event.group_id,
"organization": self.event.group.project.organization_id,
"reason": "could_not_fetch_commit_context",
"code_mappings_count": 1,
"fallback": True,
},
)

@patch(
"sentry.integrations.github.GitHubIntegration.get_commit_context",
return_value={
"commitId": "asdfasdf",
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)).strftime(
"%Y-%m-%dT%H:%M:%SZ"
),
"commitMessage": "placeholder commit message",
"commitAuthorName": "",
"commitAuthorEmail": "admin@localhost",
Expand All @@ -170,7 +213,9 @@
"sentry.integrations.github.GitHubIntegration.get_commit_context",
return_value={
"commitId": "asdfwreqr",
"committedDate": "2023-02-14T11:11Z",
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)).strftime(
"%Y-%m-%dT%H:%M:%SZ"
),
"commitMessage": "placeholder commit message",
"commitAuthorName": "",
"commitAuthorEmail": "admin@localhost",
Expand Down Expand Up @@ -255,7 +300,9 @@
"sentry.integrations.github.GitHubIntegration.get_commit_context",
return_value={
"commitId": "somekey",
"committedDate": "2023-02-14T11:11Z",
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)).strftime(
"%Y-%m-%dT%H:%M:%SZ"
),
"commitMessage": "placeholder commit message",
"commitAuthorName": "",
"commitAuthorEmail": "randomuser@sentry.io",
Expand Down Expand Up @@ -296,7 +343,9 @@
"sentry.integrations.github.GitHubIntegration.get_commit_context",
return_value={
"commitId": "somekey",
"committedDate": "2023-02-14T11:11Z",
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)).strftime(
"%Y-%m-%dT%H:%M:%SZ"
),
"commitMessage": "placeholder commit message",
"commitAuthorName": "",
"commitAuthorEmail": "randomuser@sentry.io",
Expand Down Expand Up @@ -337,7 +386,9 @@
"sentry.integrations.github.GitHubIntegration.get_commit_context",
return_value={
"commitId": "somekey",
"committedDate": "2023-02-14T11:11Z",
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)).strftime(
"%Y-%m-%dT%H:%M:%SZ"
),
"commitMessage": "placeholder commit message",
"commitAuthorName": "",
"commitAuthorEmail": "randomuser@sentry.io",
Expand Down Expand Up @@ -423,7 +474,9 @@
Mock(
return_value={
"commitId": "asdfwreqr",
"committedDate": "2023-02-14T11:11Z",
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)).strftime(
"%Y-%m-%dT%H:%M:%SZ"
),
"commitMessage": "placeholder commit message",
"commitAuthorName": "",
"commitAuthorEmail": "admin@localhost",
Expand Down
4 changes: 3 additions & 1 deletion tests/sentry/tasks/test_post_process.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@
is_regression=False,
is_new_group_environment=True,
event=event_2,
)

Check failure on line 435 in tests/sentry/tasks/test_post_process.py

View workflow job for this annotation

GitHub Actions / backend test (5, 14) 

PostProcessGroupErrorTest.test_debounce_cache_is_set

sentry.models.groupowner.GroupOwner.DoesNotExist: GroupOwner matching query does not exist.
assert MockAction.return_value.after.call_count == 1

@patch("sentry.rules.processor.RuleProcessor")
Expand Down Expand Up @@ -1296,7 +1296,9 @@
class ProcessCommitsTestMixin(BasePostProgressGroupMixin):
github_blame_return_value = {
"commitId": "asdfwreqr",
"committedDate": "",
"committedDate": (datetime.now(timezone.utc) - timedelta(days=2)).strftime(
"%Y-%m-%dT%H:%M:%SZ"
),
"commitMessage": "placeholder commit message",
"commitAuthorName": "",
"commitAuthorEmail": "admin@localhost",
Expand Down
Loading