-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
ref(sentry_apps): Rename apps.py and shim for getsentry #77614
Conversation
src/sentry/sentry_apps/apps.py
Outdated
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.
Shim
def run( | ||
self, | ||
*, | ||
user: User, |
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.
This was changed from User | RpcUser to just User. Though I wasn't 100% on this one. Like SentryApp creation and updating all happens in the CS so can we assume all 'users' won't be RpcUsers ?
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.
Like SentryApp creation and updating all happens in the CS so can we assume all 'users' won't be RpcUsers ?
We can likely use the narrower type. The RpcUser could show up in a future refactoring, but we can deal with that then.
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #77614 +/- ##
=======================================
Coverage 78.11% 78.11%
=======================================
Files 6967 6967
Lines 309330 309330
Branches 50647 50647
=======================================
+ Hits 241636 241640 +4
- Misses 61292 61294 +2
+ Partials 6402 6396 -6 |
from sentry.shared_integrations.exceptions import ApiError, IntegrationError | ||
from sentry.users.models.user import User | ||
from sentry.utils.http import absolute_uri | ||
|
||
from ...sentry_apps.apps import SentryAppCreator |
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.
🙇 Thanks for fixing this relative import.
def run( | ||
self, | ||
*, | ||
user: User, |
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.
Like SentryApp creation and updating all happens in the CS so can we assume all 'users' won't be RpcUsers ?
We can likely use the narrower type. The RpcUser could show up in a future refactoring, but we can deal with that then.
We need to rename
apps.py
so django doesn't confuse it for anAppConfig
file when we later addsentry_apps
toINSTALLED APPS.
However,apps.py
is also used ingetsentry
so we'll need to create a shim so that refs don't break.ref(#73857)