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: fetch accurate branch head #352

Merged
merged 4 commits into from
Jan 23, 2024
Merged

fix: fetch accurate branch head #352

merged 4 commits into from
Jan 23, 2024

Conversation

joseph-sentry
Copy link
Contributor

@joseph-sentry joseph-sentry commented Jan 23, 2024

Purpose/Motivation

This commit is a fix for the recent issue where
branch heads were all set to the same value each
time they were updated due to a bug in the Branch
django model.

This fix creates a helper function that checks for the 2 remaining innacurate commit shas that a large number of branches were set to. If the current branch head is set to one of those shas, we update the branch head by fetching the latest commit on that branch from the db.

Note that we must use a raw sql query here to update the branch object because using the django ORM to update it would lead to the same issue we are trying to fix.

Incident to link to

https://status.codecov.com/incidents/z10fykg3c9w4

What does this PR do?

  • create get_or_update_branch_head
  • use get_or_update_branch_head function in many endpoints most notably: graphs (badges) and the graphql branch endpoint

@joseph-sentry joseph-sentry force-pushed the joseph/temp-branch-fix branch 2 times, most recently from b126832 to 5f0cd91 Compare January 23, 2024 16:44
@joseph-sentry joseph-sentry marked this pull request as ready for review January 23, 2024 16:44
@@ -19,6 +20,8 @@ class BranchDetailSerializer(BranchSerializer):
)

def get_head_commit(self, branch: Branch) -> CommitDetailSerializer:
_ = get_or_update_branch_head(Commit.objects, branch, branch.repository_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_ = get_or_update_branch_head(Commit.objects, branch, branch.repository_id)
get_or_update_branch_head(Commit.objects, branch, branch.repository_id)

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (0cd1f15) 95.71% compared to head (0f1426f) 95.71%.

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

Files Patch % Lines
utils/temp_branch_fix.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #352   +/-   ##
=====================================
  Coverage   95.71   95.71           
=====================================
  Files        740     741    +1     
  Lines      16720   16746   +26     
=====================================
+ Hits       16002   16027   +25     
- Misses       718     719    +1     
Flag Coverage Δ
unit 96.03% <96.87%> (-0.02%) ⬇️
unit-latest-uploader 96.03% <96.87%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codecov-staging
Copy link

codecov-staging bot commented Jan 23, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Files Patch % Lines
utils/temp_branch_fix.py 90.90% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@codecov-qa
Copy link

codecov-qa bot commented Jan 23, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (0cd1f15) 96.05% compared to head (0f1426f) 96.03%.

Files Patch % Lines
utils/temp_branch_fix.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #352      +/-   ##
==========================================
- Coverage   96.05%   96.03%   -0.02%     
==========================================
  Files         625      626       +1     
  Lines       16204    16230      +26     
==========================================
+ Hits        15565    15587      +22     
- Misses        639      643       +4     
Flag Coverage Δ
unit 96.03% <96.87%> (-0.02%) ⬇️
unit-latest-uploader 96.03% <96.87%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

codecov-public-qa bot commented Jan 23, 2024

Codecov Report

Merging #352 (0f1426f) into main (0cd1f15) will decrease coverage by 0.02%.
The diff coverage is 96.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #352      +/-   ##
==========================================
- Coverage   96.05%   96.03%   -0.02%     
==========================================
  Files         625      626       +1     
  Lines       16204    16230      +26     
==========================================
+ Hits        15565    15587      +22     
- Misses        639      643       +4     
Flag Coverage Δ
unit 96.03% <96.87%> (-0.02%) ⬇️
unit-latest-uploader 96.03% <96.87%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
api/internal/coverage/views.py 100.00% <100.00%> (ø)
api/public/v2/branch/serializers.py 100.00% <100.00%> (ø)
api/shared/mixins.py 98.55% <100.00%> (+0.04%) ⬆️
graphql_api/types/branch/branch.py 100.00% <100.00%> (ø)
graphs/views.py 93.10% <100.00%> (-1.87%) ⬇️
utils/temp_branch_fix.py 90.90% <90.90%> (ø)

Impacted file tree graph

Copy link
Contributor

@giovanni-guidini giovanni-guidini left a comment

Choose a reason for hiding this comment

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

please create an issue to revisit this implementation and remove if we don't need it anymore so we don't forget about this.

@thomasrockhu-codecov
Copy link
Contributor

@joseph-sentry could you add a link to the incident perhaps in the code change? that would make future finding a little easier

@joseph-sentry
Copy link
Contributor Author

please create an issue to revisit this implementation and remove if we don't need it anymore so we don't forget about this.

codecov/engineering-team#1083

This commit is a fix for the recent issue where
branch heads were all set to the same value each
time they were updated due to a bug in the Branch
django model.

This fix creates a helper function that checks for the 2
remaining innacurate commit shas that a large number of
branches were set to. If the current branch head is set to
one of those shas, we update the branch head by fetching
the latest commit on that branch from the db.

Note that we must use a raw sql query here to update the
branch object because using the django ORM to update it
would lead to the same issue we are trying to fix.

Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
@joseph-sentry joseph-sentry merged commit bd20168 into main Jan 23, 2024
19 of 22 checks passed
@joseph-sentry joseph-sentry deleted the joseph/temp-branch-fix branch January 23, 2024 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants