-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
chore(github): Add business+ plan check for multi org #91905
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: master
Are you sure you want to change the base?
Conversation
|
FeatureDescription( | ||
""" | ||
Connect multiple Sentry organizations to a single GitHub account. | ||
""", | ||
IntegrationFeatures.SCM_MULTI_ORG, | ||
), |
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 usually feature flag adding this feature description? I was looking at some other PRs that added FeatureDescriptions
and they weren't ever feature flagged (??)
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'm okay waiting until GA to add this, since it's not available to everyone and will show up on the GH integration features page
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.
removed listing the feature in the integration page, we'll do this at GA time
@@ -936,7 +951,10 @@ def dispatch(self, request: HttpRequest, pipeline: Pipeline) -> HttpResponseBase | |||
return self.render_react_view( | |||
request=request, | |||
pipeline_name="githubInstallationSelect", | |||
props={"installation_info": installation_info}, | |||
props={ | |||
"installation_info": installation_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.
if they don't have multi org, should we still be passing the full list of installations to the FE?
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 we should also error if somebody tries to pass a chosen installation id that's not -1 and they don't have multi org
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.
For the passing the full list comment, I think it's useful to keep in case we want to do the per gh org upsell in the dropdown in the UI.
features.has( | ||
"organizations:integrations-scm-multi-org", | ||
organization=self.active_user_organization.organization, | ||
actor=request.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.
qq: whats the purpose of passing user here?
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 probably yoinked this from a previous feature flag checker, but looking further yeah we dont need to be passing the user/it wont be used.
This PR
organizations:integrations-scm-multi-org
flag to the permanent list of feature flags (there is a getsentry PR too to add the flag to business plans)chosen_installation_id
(validate the user has access to multi-org feature)Instead of having multiple conditions for the
organizations:github-multi-org
flag, we will use theorganizations:integrations-scm-multi-org
to validate that the feature can only be accessed by business+ plans andorganizations:github-multi-org
will control which orgs can access the feature.PR for FF in plans for getsentry: https://github.com/getsentry/getsentry/pull/17517
PR for UI changes: #92036