-
Notifications
You must be signed in to change notification settings - Fork 3
Add a new GraphQL mutation to sync with app token #202
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
base: main
Are you sure you want to change the base?
Conversation
Sentry detected 3 potential issues in your recent changesThere's a suspicion that the new syncRepos mutation might encounter an AttributeError if current_owner is None due to bypassed validation, potentially causing unexpected behavior.
It's suspected that the SyncReposTask, when triggered by the new syncRepos mutation, lacks error handling for NoConfiguredAppsAvailable or InvalidInstallationError from GitHub App token retrieval, potentially leading to Celery task crashes.
It is suspected that if current_owner.user is None, accessing self.current_user.is_authenticated in BaseInteractor will cause an AttributeError. This risk is heightened by the new mutation.
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. Additional details and impacted files@@ Coverage Diff @@
## main #202 +/- ##
=======================================
Coverage 94.25% 94.25%
=======================================
Files 1215 1217 +2
Lines 45114 45127 +13
Branches 1437 1437
=======================================
+ Hits 42522 42535 +13
Misses 2291 2291
Partials 301 301
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
8ac24b3
to
ac55e54
Compare
This change makes the sync task use an app token rather than the user's token. It's a separate mutation, so it shouldn't conflict with the existing sync trigger.
ac55e54
to
ff57c37
Compare
@@ -33,6 +34,7 @@ | |||
mutation = ariadne_load_local_graphql(__file__, "mutation.graphql") | |||
mutation = mutation + gql_create_api_token | |||
mutation = mutation + gql_create_stripe_setup_intent | |||
mutation = mutation + gql_sync_repos |
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.
Thoughts on a comment here to let folks know these are both triggering the sync task, one is just using integration and the other isn't?
@@ -0,0 +1,4 @@ | |||
def resolve_sync_repos(_, info): |
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 to wrap this with the error handling decorator?
umbrella/apps/codecov-api/graphql_api/helpers/mutation.py
Lines 39 to 47 in 38f9e37
def wrap_error_handling_mutation(resolver): | |
async def resolver_with_error_handling(*args, **kwargs): | |
try: | |
return await resolver(*args, **kwargs) | |
except exceptions.BaseException as e: | |
# Wrap a pure Python exception with our Wrapper to pass as a value | |
return {"error": WrappedException(e)} | |
return resolver_with_error_handling |
Like in
Line 9 in aa7bacd
@wrap_error_handling_mutation |
This change makes the sync task use an app token rather than the user's token. It's a separate mutation, so it shouldn't conflict with the existing sync trigger.
Just also note that the
current_owner
is set to be the org with the JWT middleware:umbrella/apps/codecov-api/codecov_auth/middleware.py
Line 200 in 3541fda