feat: pin down commits to apps #482
Conversation
extends NotificationResult to record the app that was used. After notifications go through we save them to `CommitNotification`, including the app. This is the "permanent pindown" Previously this would only happen for pull requests, but not it _also_ saves a `CommitNotification` everytime we have a record of an app_id being used (for a successful notification) The `Notify` task after selecting an app will possibly pindown the commit to that selected app, or refresh the permanent pindown in Redis (faster to access). Notice then, that if a notifier is to use a specific app this will certainly be in redis. Put there by the `NotifyTask`. I've found some errors in the redis get/set though (through manual testing). It turns out that the return value is a binary string, not a "regular" string. Hence the changes to the "get" method. Only notifiers that notify on a specific commit (status and checks) check for a specific app. The comment notifier might use any app. But that's OK because an app _can_ edit the comment of another app.
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #482 +/- ##
==========================================
- Coverage 97.26% 97.24% -0.03%
==========================================
Files 412 412
Lines 34150 34271 +121
==========================================
+ Hits 33217 33326 +109
- Misses 933 945 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #482 +/- ##
==========================================
- Coverage 97.26% 97.24% -0.03%
==========================================
Files 412 412
Lines 34150 34271 +121
==========================================
+ Hits 33217 33326 +109
- Misses 933 945 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #482 +/- ##
==========================================
- Coverage 97.26% 97.24% -0.03%
==========================================
Files 412 412
Lines 34150 34271 +121
==========================================
+ Hits 33217 33326 +109
- Misses 933 945 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
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 #482 +/- ##
==========================================
- Coverage 97.29% 97.27% -0.03%
==========================================
Files 443 443
Lines 34879 35000 +121
==========================================
+ Hits 33936 34045 +109
- Misses 943 955 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| pull: Pull, notifier: AbstractBaseNotifier, result_dict | ||
| comparison: ComparisonProxy, | ||
| notifier: AbstractBaseNotifier, | ||
| notification_result: NotificationResult | None, | ||
| ) -> CommitNotification: |
There was a problem hiding this comment.
From below, it looks like we can return None type as well.
| ) -> CommitNotification: | |
| ) -> CommitNotification | None: |
There was a problem hiding this comment.
good catch, thanks
| if not self._repository_service: | ||
| self._repository_service = get_repo_provider_service( | ||
| self.repository, installation_name_to_use=self.gh_installation_name | ||
| self._repository_service = get_repo_provider_service_for_specific_commit( | ||
| commit, fallback_installation_name=self.gh_installation_name | ||
| ) |
There was a problem hiding this comment.
This function also exists in StatusNotifier, which is the superclass. Can we remove the duplication?
| self._repository_service = get_repo_provider_service( | ||
| self.repository, installation_name_to_use=self.gh_installation_name | ||
| self._repository_service = get_repo_provider_service_for_specific_commit( | ||
| commit, fallback_installation_name=self.gh_installation_name | ||
| ) |
There was a problem hiding this comment.
This looks like we save the repo provider to the class object, which means that the repo service for the first commit that calls this function will be returned here for the lifecycle of StatusNotifier. Is it possible for a repo service for a commit to be stale for a subsequent commit?
There was a problem hiding this comment.
Every NotifyTask has its own NotificationService. That in turn creates new instances of all the notifiers (
worker/services/notification/__init__.py
Line 248 in 338d1c6
NotifyTask runs for a single commit, so I don't think it is possible for the commit to be stale
| return True | ||
| return False | ||
|
|
||
| def _possibly_pin_commit_to_github_app( |
There was a problem hiding this comment.
nit:
| def _possibly_pin_commit_to_github_app( | |
| def _maybe_pin_commit_to_github_app( |
| @@ -399,6 +402,55 @@ def run_impl_within_lock( | |||
| ) | |||
| return {"notified": False, "notifications": None} | |||
|
|
|||
| def _possibly_refresh_previous_selection(self, commit: Commit) -> bool: | |||
There was a problem hiding this comment.
nit:
| def _possibly_refresh_previous_selection(self, commit: Commit) -> bool: | |
| def _maybe_refresh_previous_selection(self, commit: Commit) -> bool: |
There was a problem hiding this comment.
I don't want to change this because (1) I don't feel "maybe" is an improvement to "possibly" and (2, most important) we use "possibly_" in this context in other parts of the code, so it keeps consistent
There was a problem hiding this comment.
On second thought, maybe doesn't necessarily fit here because it's more commonly used for Optional types (see monads). So I'm fine leaving it.
tasks/notify.py
Outdated
| def _possibly_pin_commit_to_github_app( | ||
| self, commit: Commit, torngit: TorngitBaseAdapter | ||
| ) -> int | str | None: | ||
| """Pins down the github app to use when emitting notifications for this commit, as needed. |
There was a problem hiding this comment.
| """Pins down the github app to use when emitting notifications for this commit, as needed. | |
| """Pin the GitHub app to use when emitting notifications for this commit, as needed. |
| @@ -103,11 +106,10 @@ def flag_coverage_was_uploaded(self, comparison) -> bool: | |||
| len(report_uploaded_flags.intersection(flags_included_in_status_check)) > 0 | |||
| ) | |||
|
|
|||
| @property | |||
| def repository_service(self): | |||
| def repository_service(self, commit: Commit): | |||
There was a problem hiding this comment.
Let's also add a type annotation for the return value and self._repository_service.
| if not self._repository_service: | ||
| self._repository_service = get_repo_provider_service( | ||
| self.repository, installation_name_to_use=self.gh_installation_name | ||
| self._repository_service = get_repo_provider_service_for_specific_commit( |
There was a problem hiding this comment.
I just want to confirm my understanding of the code: does this pull the commit specific app that was set at the beginning of the app? Specifically, _possibly_pin_commit_to_github_app sets the app to use, then this function will retrieve it if it exists.
There was a problem hiding this comment.
That's the idea, yes
extends NotificationResult to record the app that was used. After notifications go through we save them to
CommitNotification, including the app. This is the "permanent pindown"Previously this would only happen for pull requests, but not it also saves a
CommitNotificationeverytime we have a record of an app_id being used (for a successful notification)The
Notifytask after selecting an app will possibly pindown the commit to that selected app, or refresh the permanent pindown in Redis (faster to access). Notice then, that if a notifier is to use a specific app this will certainly be in redis. Put there by theNotifyTask.I've found some errors in the redis get/set though (through manual testing). It turns out that the return value is a binary string, not a "regular" string. Hence the changes to the "get" method.
Only notifiers that notify on a specific commit (status and checks) check for a specific app. The comment notifier might use any app. But that's OK because an app can edit the comment of another app.
👀 This is PR 4/4 for pinning commits to apps