feat: notify status to multiple shas (gitlab) #712
Conversation
related to codecov/engineering-team#2076 Now that we are collecting different SHAs that we potentially need to notify for Gitlab, the only missing piece was actually doing the notification in multiple SHAs. Some notes: * The `checks` notifier is exclusive for GitHub, so no need to change that * "notifying to multiple shas" is done in the most transparent way possible * I created `_send_notification_with_metrics` just to wrap the timer, but it would probably be better to add it in the Torngit adapter directly * We don't need to do this for the PR comment, just the status
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #712 +/- ##
==========================================
- Coverage 98.08% 98.07% -0.01%
==========================================
Files 434 434
Lines 36653 36776 +123
==========================================
+ Hits 35950 36069 +119
- Misses 703 707 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #712 +/- ##
==========================================
- Coverage 98.08% 98.07% -0.01%
==========================================
Files 434 434
Lines 36653 36776 +123
==========================================
+ Hits 35950 36069 +119
- Misses 703 707 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #712 +/- ##
==========================================
- Coverage 98.08% 98.07% -0.01%
==========================================
Files 434 434
Lines 36653 36776 +123
==========================================
+ Hits 35950 36069 +119
- Misses 703 707 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Changes have been made to critical files, which contain lines commonly executed in production. Learn more ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #712 +/- ##
========================================
Coverage 98.12% 98.13%
========================================
Files 475 475
Lines 38008 38382 +374
========================================
+ Hits 37297 37667 +370
- Misses 711 715 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes
|
| @@ -273,7 +272,7 @@ def get_status_external_name(self) -> str: | |||
| return f"codecov/{self.context}{status_piece}" | |||
|
|
|||
| async def maybe_send_notification( | |||
| self, comparison: Comparison, payload: dict | |||
| self, comparison: ComparisonProxy, payload: dict | |||
There was a problem hiding this comment.
Has this always been Proxy and we never realized?
There was a problem hiding this comment.
I think so... Looking at the data flow in the code this does seem to be the case
| description=message, | ||
| url=url, | ||
| ) | ||
| all_results = await asyncio.gather(*all_notify_tasks) |
There was a problem hiding this comment.
Is there any overhead of doing gather vs await if all_notify_tasks has more often than not length = 1?
There was a problem hiding this comment.
I don't know. But this seems to be the best way to do it.
It probably has some overhead, but I doubt it's relevant
| @@ -336,26 +357,37 @@ async def send_notification(self, comparison: Comparison, payload): | |||
| "state": state, | |||
| "message": message, | |||
| } | |||
|
|
|||
| all_shas_to_notify = [head_commit_sha] + list( | |||
| comparison.context.gitlab_extra_shas or set() | |||
There was a problem hiding this comment.
Optional: leave a comment explaining why we have to do this, also optional since you already left a comment in the gitlab_extra_shas definition
There was a problem hiding this comment.
I was hoping that having a comment both in the gitlab_extra_shas definition and the function that calculates those extra shas would suffice.
| url=url, | ||
| ) | ||
| all_results = await asyncio.gather(*all_notify_tasks) | ||
| res = all_results[0] |
There was a problem hiding this comment.
Would we only care of the 1st result here? I guess cause the second+ would be the extra gitlab ones?
There was a problem hiding this comment.
Exactly. The 1st result matches the notification we were doing before. All others should be as transparent as possible, I suppose
related to codecov/engineering-team#2076
Now that we are collecting different SHAs that we potentially need to notify for Gitlab,
the only missing piece was actually doing the notification in multiple SHAs.
Some notes:
checksnotifier is exclusive for GitHub, so no need to change that_send_notification_with_metricsjust to wrap the timer, but it would probably be better toadd it in the Torngit adapter directly