feat: use GithubAppInstalltion in the worker #239
Conversation
giovanni-guidini
requested review from
adrian-codecov,
joseph-sentry and
matt-codecov
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 Additional details and impacted files@@ Coverage Diff @@
## main #239 +/- ##
==========================================
+ Coverage 98.06% 98.07% +0.01%
==========================================
Files 406 406
Lines 31545 31714 +169
==========================================
+ Hits 30936 31105 +169
Misses 609 609
Flags with carried forward coverage won't be shown. Click here to find out more.
|
giovanni-guidini
force-pushed
the
gio/multi-apps/model
branch
from
e15d378 to
8030169
Compare
giovanni-guidini
force-pushed
the
gio/multi-apps/usage
branch
from
1e19cf8 to
b2561e3
Compare
giovanni-guidini
force-pushed
the
gio/multi-apps/model
branch
from
8030169 to
a062340
Compare
giovanni-guidini
force-pushed
the
gio/multi-apps/usage
branch
from
b2561e3 to
e2731f9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## main #239 +/- ##
==========================================
+ Coverage 98.09% 98.10% +0.01%
==========================================
Files 375 375
Lines 30844 31013 +169
==========================================
+ Hits 30256 30425 +169
Misses 588 588
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 ✅
Additional details and impacted files@@ Coverage Diff @@
## main #239 +/- ##
==========================================
+ Coverage 98.09% 98.10% +0.01%
==========================================
Files 375 375
Lines 30844 31013 +169
==========================================
+ Hits 30256 30425 +169
Misses 588 588
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov Report
@@ Coverage Diff @@
## main #239 +/- ##
==========================================
+ Coverage 98.09% 98.10% +0.01%
==========================================
Files 375 375
Lines 30844 31013 +169
==========================================
+ Hits 30256 30425 +169
Misses 588 588
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -179,7 +180,24 @@ async def run_async_within_lock( | |||
| "Not sending notifications yet because we are waiting for CI to finish", | |||
| extra=dict(repoid=commit.repoid, commit=commit.commitid), | |||
| ) | |||
| if commit.repository.using_integration or commit.repository.hookid: | |||
| ghapp_default_installations = list( | |||
There was a problem hiding this comment.
I see this affects the notify task for coverage uploads. Would it be tough to add the GH App integration for the bundle analysis notifier?
worker/services/bundle_analysis.py
Line 268 in c248c6f
There was a problem hiding this comment.
(If unfamiliar, BA uses a different notifier cause the normal notifier relied too much on coverage data being present, hence Scott opted to do a separate notifier wrapper class)
There was a problem hiding this comment.
When making these changes I looked for references of .using_integration. There was no such ref in the bundle analysis notifier.
I guess it was decided that that task doesn't need to check this for the timeout on the retry. Personally I think that this implementation here is a bit "too much", but was respecting the logic that's in place.
| # Getting owner installation - not tied to any particular repo | ||
| return app_installation.installation_id | ||
| # DEPRECATED FLOW - begin | ||
| if owner.integration_id and deprecated_using_integration: |
There was a problem hiding this comment.
Could this code run? How do customers with their integration id tied to their owner start using the new GH App class? Would that be "auto-populated" after syncing?
There was a problem hiding this comment.
Yes, the code can run if there's no GithubAppInstallation object for a given owner.
The GH App objects are populated by the API when we receive the installation webhook.
I don't think there are plans for backfilling this info, so it would only be filled for an existing user if they make changes to their installation.
| # DEPRECATED FLOW - end | ||
| return None | ||
|
|
||
|
|
||
| def get_repo_appropriate_bot_token(repo: Repository) -> Tuple[Dict, Optional[Owner]]: |
There was a problem hiding this comment.
Echoing same thoughts as the other PR, is there a way to test this locally/staging?
| ) | ||
| # filter is an Iterator, so we need to scan matches | ||
| # in practice we only consider the 1st one | ||
| for app_installation in default_app_installation_filter: |
There was a problem hiding this comment.
will this always contain one or zero installations? or is there a possibility of there being multiple?
There was a problem hiding this comment.
Currently there will always be 0 or 1 installations I believe. That comes from Github when the user installs the codecov gh app to their profile. I don't think it's possible for a user in GH to have multiple installations of the same app in their profile.
Assume in the future there are many such installations for some reason.
The idea of installation_name is to identify app's functionalities in such a way that they are interchangeable.
Although we'd probably want to be able to pass a list of names and select one option randomly... but that's for the future and there might be other implementation choices available.
depends on: #236 context: codecov/engineering-team#970 usage of the `GithubAppInstallation` model through the worker. It still respects and doesn't affect deprecated usages of `owner.integration_id` and `repo.using_integration`, but if a ghapp exists for the owner it takes precedence over the legacy fields.
giovanni-guidini
force-pushed
the
gio/multi-apps/usage
branch
from
d4c4a7b to
a2016bc
Compare
depends on: #236
context: codecov/engineering-team#970
usage of the
GithubAppInstallationmodel through the worker.It still respects and doesn't affect deprecated usages of
owner.integration_idandrepo.using_integration, but if aghapp exists for the owner it takes precedence over the legacy fields.
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.