-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(github): allow users to uninstall github from Sentry #21327
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
Conversation
| integration_cls = GitHubIntegration | ||
| features = frozenset([IntegrationFeatures.COMMITS, IntegrationFeatures.ISSUE_BASIC]) | ||
|
|
||
| can_disable = True |
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'd explicitly set can_disable to false and leave comment describing the user issue.
Also the comment above IntegrationProvider.can_disable doesn't really describe what disabling does.
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 agree it's a little confusing because technically the integration can still be set to disabled if the user uninstalls from GH first. But we use this attribute for whether or not we show the disable dialog text.
I think we should really be documenting the install and uninstall flows for our integrations and related data (i.e. repos, commits, alert rules)
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.
@MeredithAnya @mgaeta I added a new comment in the base class what can_disable actually does
|
For anyone who is curious, the sentry/src/sentry/tasks/deletion.py Lines 379 to 381 in 3eeb60e
The reason the repos get reattached is that we added the
|
MeredithAnya
left a comment
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 wish we could still link people to GH so that they uninstall over there. But not sure where that would go. And maybe we just see if that is even a problem
Based on my testing, it's rather unnecessary IMO. Uninstalling from Github's side doesn't really provide any benefits. |
Historically, we have prevented users from uninstalling Github from Sentry because there is still an installation on the Github side. We forced users to uninstall from Github which triggers a webhook to disable the integration in Sentry. Only at that point could a user remove the integration. However, this is proving problematic because sometimes integrations get in a bad state and it would be better to just remove the installation (and sometimes reinstall).
In the new flow, users can always uninstall the Github integration from Sentry. When that happens, the app is still installed on Github's side but we remove the
OrganizationIntegrationrow and remove theintegration_idon the attached repos. If the user wants to reinstall then they would uninstall and reinstall in Github. Those repos which were unattached automatically get reattached to the new integration. So there isn't really a downside to letting the user uninstall in Sentry (things won't get into a bad state).I've also fixed a
KeyErrorin Sentry when the webhook tries to remove an integration that doesn't exist (SENTRY-CF1).Fixes: #20599