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

fix(commit-context): do not create suspect commits older than 1 year #55013

Merged
merged 13 commits into from
Aug 28, 2023

Conversation

NisanthanNanthakumar
Copy link
Contributor

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.

@NisanthanNanthakumar NisanthanNanthakumar requested a review from a team as a code owner August 18, 2023 06:03
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 18, 2023
@NisanthanNanthakumar NisanthanNanthakumar changed the title Fix/do not create suspect commits older than 1 year fix(commit-context): do not create suspect commits older than 1 year Aug 18, 2023
@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Merging #55013 (6e9972a) into master (b3cd0eb) will decrease coverage by 0.03%.
Report is 61 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #55013      +/-   ##
==========================================
- Coverage   79.95%   79.92%   -0.03%     
==========================================
  Files        5034     5035       +1     
  Lines      214531   214862     +331     
  Branches    36444    36502      +58     
==========================================
+ Hits       171521   171739     +218     
- Misses      37741    37849     +108     
- Partials     5269     5274       +5     
Files Changed Coverage Δ
src/sentry/integrations/github/integration.py 83.06% <100.00%> (+4.05%) ⬆️
src/sentry/integrations/gitlab/integration.py 87.50% <100.00%> (ø)
src/sentry/integrations/utils/commit_context.py 80.00% <100.00%> (+1.42%) ⬆️

... and 162 files with indirect coverage changes

"event": self.event.event_id,
"group": self.event.group_id,
"organization": self.event.group.project.organization_id,
"reason": "could_not_fetch_commit_context",
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense for the reason to be more specific? e.g. "commit_is_older_than_a_year"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ehh it could be that only one of the code mappings/installations had the old commit. find_commit_context_for_event is a function that will return a list of commit contexts. I think this is valid when there is only 1 code mapping and that resulted in the old commit.

"organization": self.event.group.project.organization_id,
"reason": "could_not_fetch_commit_context",
"code_mappings_count": 1,
"fallback": True,
Copy link
Member

Choose a reason for hiding this comment

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

Does the fallback mean that we would use the previous version of suspect commits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

return {
"commitId": commitInfo.get("oid"),
"committedDate": commitInfo.get("committedDate"),
"committedDate": committed_date,
Copy link
Member

Choose a reason for hiding this comment

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

According to codecov, the block above is not being tested. Please look into it before we merge this.
image

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 a test to increase coverage.

Copy link
Member

@armenzg armenzg left a comment

Choose a reason for hiding this comment

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

Thanks for doing so!

@snigdhas snigdhas merged commit 6544999 into master Aug 28, 2023
58 checks passed
@snigdhas snigdhas deleted the fix/do-not-create-suspect-commits-older-than-1-year branch August 28, 2023 19:31
@github-actions github-actions bot locked and limited conversation to collaborators Sep 13, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants