feat: Notify that commit is currently processing #179
Conversation
Codecov Report
Changes have been made to critical files, which contain lines commonly executed in production. Learn more @@ Coverage Diff @@
## main #179 +/- ##
=====================================
Coverage 95.51 95.51
=====================================
Files 715 715
Lines 15646 15664 +18
=====================================
+ Hits 14943 14961 +18
Misses 703 703
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
=======================================
Coverage 95.60% 95.61%
=======================================
Files 601 602 +1
Lines 15240 15267 +27
=======================================
+ Hits 14570 14597 +27
Misses 670 670
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov Report
@@ Coverage Diff @@
## main #179 +/- ##
=======================================
Coverage 95.60% 95.61%
=======================================
Files 601 602 +1
Lines 15240 15267 +27
=======================================
+ Hits 14570 14597 +27
Misses 670 670
Flags with carried forward coverage won't be shown. Click here to find out more.
|
scott-codecov
force-pushed
the
scott/notify-upload-processing
branch
from
7cdaf54
to
840aa18
Compare
|
Reworked some of this so requested reviews again |
| "app.tasks.notify.Notify", | ||
| kwargs=dict( | ||
| repoid=repoid, | ||
| commitid=commitid, | ||
| current_yaml=current_yaml, | ||
| empty_upload=empty_upload, | ||
| ), | ||
| ) | ||
|
|
||
| def notify(self, repoid, commitid, current_yaml=None, empty_upload=None): |
There was a problem hiding this comment.
since now we're using different kinds of notifications here, can we rename the empty_upload to be something more descriptive? like notification_type for example?
There was a problem hiding this comment.
Yeah I think that would make sense. I can update it in the worker too but will have to support both empty_upload and notification_type for a little bit so as to preserve backward compatibility.
| @@ -34,7 +34,7 @@ | |||
|
|
|||
|
|
|||
| class TaskService(object): | |||
| def _create_signature(self, name, args=None, kwargs=None): | |||
| def _create_signature(self, name, args=None, kwargs=None, immutable=False): | |||
There was a problem hiding this comment.
this is for my own understanding, what's the immutable for?
There was a problem hiding this comment.
Yeah, I had to look this up in the Celery docs. When we use Celery's chain or link then the results of the previous task are passed as arguments to the next task in the chain (this is the default and is when immutable=False). I left a small comment about this where I pass immutable=True and the goal was to prevent the results of the notify task from being passed as args to the upload task.
scott-codecov
force-pushed
the
scott/notify-upload-processing
branch
from
820dddd
to
46d8520
Compare
Codecov ReportAttention:
📢 Thoughts on this report? Let us know!. |
upload/helpers.py
Outdated
| ) | ||
|
|
||
| notified = CommitNotification.objects.filter(commit__commitid=commitid).exists() |
There was a problem hiding this comment.
I'm realizing we probably need to store this per-PR since when we make another commit on the same branch we don't necessarily want to show the "processing" comment again. 🤔
| repository_id=commit.repository_id, pullid=commit.pullid | ||
| ).first() | ||
| if pull and pull.commentid: | ||
| notified = True |
There was a problem hiding this comment.
@dana-yaish What do you think about this approach? I had been checking CommitNotification previously but that resulted in us showing the "processing" comment for a brief period each time a new commit was pushed to a PR.
There was a problem hiding this comment.
Isn't that more accurate though? Every time I push a new commit in a PR I expect the CI to run again and update me of the status of the new commit.
If we had 2 slightly different versions of "we are processing the upload" and "you updated this PR and we are processing the upload" that seems the most accurate to me.
It somehow seems better than the comment with the red banner saying "the head of this PR doesn't match these calculations, please upload new coverage"
Purpose/Motivation
Send an early notification that an upload is being processed.
Depends on codecov/worker#140
Links to relevant tickets
codecov/engineering-team#553
What does this PR do?
Enqueues a notify task as soon as we receive an upload (but before it's processed). This notification will issue a PR comment that says something like "We're processing your upload".