fallback to assuming repo is covered #406
Conversation
I suspect that we might have a little bug in some situations when syncing repos using integration. Let's assume that suspicion is true and we might have repos not properly sinced in the installation. These changes aim to not break the app in a specific scenario: - Org installs Codecov App (not selecting all repos) - Org admin logs in and sets up a new repo - For some reason we fail to sync the repo and the installation list of repos. Maybe we missed a webhook? Would be the only possibility really... but maybe the admin logging in would actually make the repo appear in the DB, even missing the webhook. - App is still broken cause the ORG never logged in, and the repo is not marked as "covered" by the installation The changes here would fix that scenario by ignoring the repo when selecting the token to communicate with GH. Ideally we fix the sync process and remove this fix, of course. Notice that if the owner HAS a token or bot than we use that instead.
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: services/repository.py
Did you find this useful? React with a 👍 or 👎 |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #406 +/- ##
=======================================
Coverage 97.49% 97.49%
=======================================
Files 395 395
Lines 33370 33393 +23
=======================================
+ Hits 32533 32556 +23
Misses 837 837
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 Additional details and impacted files@@ Coverage Diff @@
## main #406 +/- ##
=======================================
Coverage 97.49% 97.49%
=======================================
Files 395 395
Lines 33370 33395 +25
=======================================
+ Hits 32533 32558 +25
Misses 837 837
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 #406 +/- ##
=======================================
Coverage 97.49% 97.49%
=======================================
Files 395 395
Lines 33370 33395 +25
=======================================
+ Hits 32533 32558 +25
Misses 837 837
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 #406 +/- ##
=======================================
Coverage 97.51% 97.51%
=======================================
Files 426 426
Lines 34070 34093 +23
=======================================
+ Hits 33223 33246 +23
Misses 847 847
Flags with carried forward coverage won't be shown. Click here to find out more.
|
services/repository.py
Outdated
| ignore_installation=False, | ||
| installation_name=installation_name_to_use, | ||
| ) | ||
| print("INSTALLATION INFO (no repo)", installation_info_without_considering_repo) |
There was a problem hiding this comment.
the content in these print statements can still remain, but they should probably be log.info
There was a problem hiding this comment.
Nah, I forgot to remove those :E
| repository, installation_info | ||
| ) | ||
| except RepositoryWithoutValidBotError: | ||
| # GitHub exclusive bit -- START |
There was a problem hiding this comment.
should we check owner.service?
There was a problem hiding this comment.
It's not a problem if we don't check cause it would just raise the exception again, but yeah, makes sense to do that as save a few operations... and have a clearer distinction of logic in the code.
| fallback_installations=( | ||
| installation_info.get("fallback_installations") | ||
| if installation_info | ||
| else None |
There was a problem hiding this comment.
So is to have a fallback option, the gh app we think doesn't have that repo selected correct?
If there is a fallback, and it indeed doesn't have the repo in that fallback, would it eventually error out?
There was a problem hiding this comment.
This was a lint change. It's not directly related to the change here.
We only have fallback installations if the user in question has multiple configured apps (dedicated cloud). In this case you would have installations that cover the repo (more than 1 actually) and not raise RepoBotNotConfigured.
There was a problem hiding this comment.
Oh I see that makes sense, okok ty
|
Now that the sync_repos bug is fixed and we ran the backfill task for gh apps this seems unecessary |
I suspect that we might have a little bug in some situations when syncing repos using integration.
Let's assume that suspicion is true and we might have repos not properly sinced in the installation.
These changes aim to not break the app in a specific scenario:
The changes here would fix that scenario by ignoring the repo when selecting the token to communicate with GH.
Ideally we fix the sync process and remove this fix, of course.
Notice that if the owner HAS a token or bot than we use that instead.
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.