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.

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

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

Conversation

NisanthanNanthakumar
Copy link
Contributor

Objective

Update on the reverted PR: #54624

The original PR had errors from GitLab date strings not being parsed correctly bc they are formatted differently from GitHub. So this PR returns the datetime object for internal usage and will make date comparisons easier without needing to know which integration it came from.

@NisanthanNanthakumar NisanthanNanthakumar requested a review from a team as a code owner August 16, 2023 18:36
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 16, 2023
Comment on lines 184 to 186
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.

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

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #54866 (81b93a7) into master (3af433b) will increase coverage by 0.11%.
Report is 207 commits behind head on master.
The diff coverage is 90.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #54866      +/-   ##
==========================================
+ Coverage   79.74%   79.86%   +0.11%     
==========================================
  Files        4995     5025      +30     
  Lines      212230   213885    +1655     
  Branches    36179    36393     +214     
==========================================
+ Hits       169247   170813    +1566     
- Misses      37775    37833      +58     
- Partials     5208     5239      +31     
Files Changed Coverage Δ
src/sentry/integrations/github/integration.py 78.68% <66.66%> (-0.32%) ⬇️
src/sentry/integrations/gitlab/integration.py 87.50% <100.00%> (ø)
src/sentry/integrations/utils/commit_context.py 80.00% <100.00%> (+1.42%) ⬆️

... and 187 files with indirect coverage changes

@NisanthanNanthakumar NisanthanNanthakumar merged commit 5e70962 into master Aug 17, 2023
58 checks passed
@NisanthanNanthakumar NisanthanNanthakumar deleted the do-not-create-suspect-commits-older-than-1-year-old branch August 17, 2023 14:07
@sentry-io
Copy link

sentry-io bot commented Aug 17, 2023

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ AttributeError: 'NoneType' object has no attribute 'split' sentry.tasks.process_commit_context View Issue

Did you find this useful? React with a 👍 or 👎

@asottile-sentry asottile-sentry added the Trigger: Revert add to a merged PR to revert it (skips CI) label Aug 17, 2023
@getsentry-bot
Copy link
Contributor

PR reverted: 245bce9

getsentry-bot added a commit that referenced this pull request Aug 17, 2023
…54866)"

This reverts commit 5e70962.

Co-authored-by: asottile-sentry <103459774+asottile-sentry@users.noreply.github.com>
@asottile-sentry
Copy link
Member

@NisanthanNanthakumar please write a test for this. this has now been reverted twice and the codecov integration has pointed out that this change is not covered by tests

snigdhas added a commit that referenced this pull request Aug 28, 2023
…55013)

## Objective:

Update on the reverted PR:
#54866

Fixes SENTRY-14ZG

The problem was the wrong key. copy-paste error.

But I noticed from the captured exceptions that some of the results from
the github API does not return a date for the commit. So I added a
filter for to exclude those commits from the `max` function that
determines the most recent commit.

---------

Co-authored-by: Snigdha Sharma <snigdha.sharma@sentry.io>
Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Sep 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components Trigger: Revert add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants