fix: make sure to update commit info #158
Conversation
The pre-process commit task doesn't check if the commit data has been already pulled from the provider. This can cause an error where the parent commit is not found to carryforward the reports. These changes make sure that we try to update the commit info if necessary. It also moves the function - that was repeated in a few places - to RepositoryService.
giovanni-guidini
force-pushed
the
gio/possibly-update-commit-before-carryforward-reports
branch
from
ad2d337
to
01473f1
Compare
giovanni-guidini
requested review from
dana-yaish,
scott-codecov and
matt-codecov
Codecov Report
@@ Coverage Diff @@
## main #158 +/- ##
==========================================
- Coverage 98.39% 98.38% -0.01%
==========================================
Files 348 348
Lines 27464 27451 -13
==========================================
- Hits 27022 27009 -13
Misses 442 442
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #158 +/- ##
==========================================
- Coverage 98.39% 98.38% -0.01%
==========================================
Files 348 348
Lines 27464 27451 -13
==========================================
- Hits 27022 27009 -13
Misses 442 442
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov Report
@@ Coverage Diff @@
## main #158 +/- ##
==========================================
- Coverage 98.39% 98.38% -0.01%
==========================================
Files 348 348
Lines 27464 27451 -13
==========================================
- Hits 27022 27009 -13
Misses 442 442
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov Report
@@ Coverage Diff @@
## main #158 +/- ##
==========================================
- Coverage 98.35% 98.35% -0.01%
==========================================
Files 374 374
Lines 27960 27947 -13
==========================================
- Hits 27499 27486 -13
Misses 461 461
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Change looks good. I think we have a more general issue w/ our architecture around this though - just sharing some thoughts here.
The preprocess commit task is enqueued during the create-report stage of an upload. The task gets put on a queue and a subsequent do-upload is run (which then enqueues an upload task). Say the preprocess task gets popped by a worker that is slow to process the task ahead of it (tasks are prefetched in bulk). Then the upload task might execute before preprocess and we have the same issue. Granted this is probably not going to happen very often but I think we are not strictly enforcing the order of execution when we don't use Celery's chain functionality.
Am I overthinking this?
You are not overthinking. But I'd say strictly speaking we cannot assume a task will run before another. That is part of the celery architecture. But the upload task does have these same checks in place, so we shouldn't have problems. |
giovanni-guidini
deleted the
gio/possibly-update-commit-before-carryforward-reports
branch
Not even when using |
|
Oh definitely you are right about |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
The pre-process commit task doesn't check if the commit data has
been already pulled from the provider. This can cause an error where
the parent commit is not found to carryforward the reports.
These changes make sure that we try to update the commit info if necessary.
It also moves the function - that was repeated in a few places - to
RepositoryService.
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.