-
Notifications
You must be signed in to change notification settings - Fork 11
add get_repo_provider_service fn to base task #1107
Conversation
83084f5
to
61a4253
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle this looks good to me. I also think this should apply to get_owner_provider_service
as it also calls the same fn. I believe Gio worked on this fn a while back so might be worth getting his opinion too
c41ea20
to
160cc32
Compare
we can burn that bridge when we come to it haha |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #1107 +/- ##
==========================================
- Coverage 97.25% 97.25% -0.01%
==========================================
Files 454 454
Lines 37335 37392 +57
==========================================
+ Hits 36310 36365 +55
- Misses 1025 1027 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
countdown_seconds=retry_delay_seconds, | ||
), | ||
) | ||
self._retry(countdown=retry_delay_seconds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not a big fan of putting more methods onto the "celery task", but instead move more code out of the "celery as task scheduling framework".
I think the only reason for this to be tied to "celery as a framework" is converting the exception to a retry.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
get_repo_provider_service()
can raise two exceptions which we do not consistently catch. from a quick scan of usages, it appears to make sense to handle them the same way across tasks, so this PR adds a wrapper method toBaseCodecovTask
that behaves as follows:get_repo_provider_service()
RepositoryWithoutValidBotError
, callsave_commit_error()
and returnNone
. each task can decide whether it can proceed without arepo_provider
or notNoConfiguredAppsAvailable
, there are two ways to handle it:exc.rate_limited_count > 0
, retry the task at the start of the next hourNone
and each task can decide whether it can proceed without arepo_provider
or noti created codecov/engineering-team#3389 to keep track of moving usages of
get_repo_provider_service()
to this new method since there are many usages to move and some of them may prove to be non-trivial