-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: GithubAppInstallation model #346
Conversation
Uses the gh app installation WITHOUT compromising the old usage (of `owner.integration_id`). Deprecated paths are marked so. These changes don't backfill old data, but start using the new model with webhooks. depends on: #346
32cbe7e
to
e0cb312
Compare
Uses the gh app installation WITHOUT compromising the old usage (of `owner.integration_id`). Deprecated paths are marked so. These changes don't backfill old data, but start using the new model with webhooks. depends on: #346
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #346 +/- ##
=======================================
Coverage 96.06% 96.07%
=======================================
Files 624 625 +1
Lines 16171 16196 +25
=======================================
+ Hits 15535 15560 +25
Misses 636 636
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 #346 +/- ##
=======================================
Coverage 96.06% 96.07%
=======================================
Files 624 625 +1
Lines 16171 16196 +25
=======================================
+ Hits 15535 15560 +25
Misses 636 636
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Model definitions for GithubAppInstallation. This will unlock multiple apps for an org. It also will improve how we keep track of what repos an installation can access. context: codecov/engineering-team#969
e0cb312
to
a58ea09
Compare
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 #346 +/- ##
=======================================
- Coverage 95.69 95.67 -0.02
=======================================
Files 738 740 +2
Lines 16670 16842 +172
=======================================
+ Hits 15951 16112 +161
- Misses 719 730 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Uses the gh app installation WITHOUT compromising the old usage (of `owner.integration_id`). Deprecated paths are marked so. These changes don't backfill old data, but start using the new model with webhooks. the sample data from tests (the keys) are taken from [github docs](https://docs.github.com/en/webhooks/webhook-events-and-payloads#installation) and some amount of empirical data. depends on: #346
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like the helper methods, nice touch
pls address comments but this'll defo do the trick
GITHUB_APP_INSTALLATION_DEFAULT_NAME = "codecov_app_installation" | ||
|
||
|
||
class GithubAppInstallation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we also need an app id column? since we may use sentry's app or support multiple apps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the "installation_id" is the "app_id"
# replacement for owner.integration_id | ||
# installation id GitHub sends us in the installation-related webhook events | ||
installation_id = models.IntegerField(null=False, blank=False) | ||
name = models.TextField(default=GITHUB_APP_INSTALLATION_DEFAULT_NAME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is what will differentiate the type of apps. As you mentioned above, we might use sentry's app or support multiple apps. This is how we know which is which.
Uses the gh app installation WITHOUT compromising the old usage (of `owner.integration_id`). Deprecated paths are marked so. These changes don't backfill old data, but start using the new model with webhooks. the sample data from tests (the keys) are taken from [github docs](https://docs.github.com/en/webhooks/webhook-events-and-payloads#installation) and some amount of empirical data. depends on: #346
Uses the gh app installation WITHOUT compromising the old usage (of `owner.integration_id`). Deprecated paths are marked so. These changes don't backfill old data, but start using the new model with webhooks. the sample data from tests (the keys) are taken from [github docs](https://docs.github.com/en/webhooks/webhook-events-and-payloads#installation) and some amount of empirical data. depends on: #346
Uses the gh app installation WITHOUT compromising the old usage (of `owner.integration_id`). Deprecated paths are marked so. These changes don't backfill old data, but start using the new model with webhooks. the sample data from tests (the keys) are taken from [github docs](https://docs.github.com/en/webhooks/webhook-events-and-payloads#installation) and some amount of empirical data. depends on: #346
Uses the gh app installation WITHOUT compromising the old usage (of `owner.integration_id`). Deprecated paths are marked so. These changes don't backfill old data, but start using the new model with webhooks. the sample data from tests (the keys) are taken from [github docs](https://docs.github.com/en/webhooks/webhook-events-and-payloads#installation) and some amount of empirical data. depends on: #346
Uses the gh app installation WITHOUT compromising the old usage (of `owner.integration_id`). Deprecated paths are marked so. These changes don't backfill old data, but start using the new model with webhooks. the sample data from tests (the keys) are taken from [github docs](https://docs.github.com/en/webhooks/webhook-events-and-payloads#installation) and some amount of empirical data. depends on: #346
Uses the gh app installation WITHOUT compromising the old usage (of `owner.integration_id`). Deprecated paths are marked so. These changes don't backfill old data, but start using the new model with webhooks. the sample data from tests (the keys) are taken from [github docs](https://docs.github.com/en/webhooks/webhook-events-and-payloads#installation) and some amount of empirical data. depends on: #346
These changes port codecov/codecov-api#346 to the worker so we can use them here as well. context: codecov/engineering-team#970
These changes port codecov/codecov-api#346 to the worker so we can use them here as well. context: codecov/engineering-team#970
* feat: use GithubAppInstallation in api Uses the gh app installation WITHOUT compromising the old usage (of `owner.integration_id`). Deprecated paths are marked so. These changes don't backfill old data, but start using the new model with webhooks. the sample data from tests (the keys) are taken from [github docs](https://docs.github.com/en/webhooks/webhook-events-and-payloads#installation) and some amount of empirical data. depends on: #346 * refactor webhook handling related to gh app installation * use ghapp when getting torngit adapter Considers ghapp installations when deciding if a torngit adapter is for a repo with integration or not. If a ghapp installation exist then repo is considered to be using integration if it's covered by the installation. Otherwise previous behavior is used. The test `test_get_adapter_sets_token_to_bot_when_user_not_authenticated` includes `owner` as `None` when calling `RepoProviderService.get_adapter` so I'm assuming that can happen, and changing the typehint in the function. Other functions that were typehinted with `Owner` instead of `Optional[Owner]` are also changed.
These changes port codecov/codecov-api#346 to the worker so we can use them here as well. context: codecov/engineering-team#970
These changes port codecov/codecov-api#346 to the worker so we can use them here as well. context: codecov/engineering-team#970
Model definitions for GithubAppInstallation.
This will unlock multiple apps for an org.
It also will improve how we keep track of what repos
an installation can access.
context: codecov/engineering-team#969
this is PR 1 of 2