feat: get repo service for specific commit #479
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #479 +/- ##
=======================================
Coverage 97.26% 97.26%
=======================================
Files 412 412
Lines 34187 34150 -37
=======================================
- Hits 33252 33217 -35
+ Misses 935 933 -2
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 #479 +/- ##
=======================================
Coverage 97.26% 97.26%
=======================================
Files 412 412
Lines 34187 34150 -37
=======================================
- Hits 33252 33217 -35
+ Misses 935 933 -2
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 #479 +/- ##
=======================================
Coverage 97.26% 97.26%
=======================================
Files 412 412
Lines 34187 34150 -37
=======================================
- Hits 33252 33217 -35
+ Misses 935 933 -2
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 #479 +/- ##
==========================================
+ Coverage 97.29% 97.30% +0.01%
==========================================
Files 443 443
Lines 34916 35102 +186
==========================================
+ Hits 33972 34157 +185
- Misses 944 945 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes
|
giovanni-guidini
force-pushed
the
gio/pin-commits/repo-service-for-commit
branch
from
ccc968c
to
1ed190e
Compare
Change the repository service so we have a function that checks whether a commit has been pinned to a specific app and returns the TorngitAdapter with said app.
giovanni-guidini
force-pushed
the
gio/pin-commits/repo-service-for-commit
branch
from
1ed190e
to
b8dcb6f
Compare
| if installation_for_commit is None: | ||
| return get_repo_provider_service(repository, fallback_installation_name) | ||
|
|
||
| ghapp_details = get_specific_github_app_details( |
There was a problem hiding this comment.
Looking at the logic inside here,
worker/services/bots/github_apps.py
Line 163 in b8dcb6f
services/repository.py
Outdated
| commit: Commit, | ||
| fallback_installation_name: str = GITHUB_APP_INSTALLATION_DEFAULT_NAME, | ||
| ) -> torngit.base.TorngitBaseAdapter: | ||
| """Gets a Torngit adapter (potentially) using a specific github app as the authentication source. |
There was a problem hiding this comment.
| """Gets a Torngit adapter (potentially) using a specific github app as the authentication source. | |
| """Gets a Torngit adapter (potentially) using a specific GitHub app as the authentication source. |
services/repository.py
Outdated
| @@ -65,7 +97,9 @@ def get_repo_provider_service( | |||
| data = TorngitInstanceData( | |||
| repo=RepoInfo( | |||
| name=repository.name, | |||
| using_integration=_is_repo_using_integration(repository), | |||
| using_integration=( | |||
| adapter_auth_info["selected_installation_info"] is not None | |||
There was a problem hiding this comment.
This will raise a KeyError if selected_installation_info is not found. If we don't want to error, then adapter_auth_info.get("selected_installation_info") is safer.
| if installation_for_commit is None: | ||
| return get_repo_provider_service(repository, fallback_installation_name) | ||
|
|
||
| ghapp_details = get_specific_github_app_details( |
There was a problem hiding this comment.
We do a check in get_repo_provider_service to ensure that the repo is a GitHub repo. Should we do a check here as well? This will save us a database read to the GitHub App Installation table I think.
In fact, many parts of this function is GitHub specific... can we skip some more steps below if it's not a GitHub commit?
There was a problem hiding this comment.
This check is in get_github_app_for_commit.
From the docstring: "If the commit doesn't have a particular app assigned to it, return regular get_repo_provider_service choice".
Because apps only exist for GitHub it follows that only if it's a GitHub repo the code would get to this point.
Change the repository service so we have a function that
checks whether a commit has been pinned to a specific app
and returns the TorngitAdapter with said app.
Note on
services.repository._is_repo_using_integration: this function has become effectively obsolete since the introduction ofget_adapter_auth_informationbecause we can tell if we are using the installation by the presence of "installation_info" or not.👀 This is PR 3/4 in the pinning commits to apps endeavour
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.