-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Integrations support self hosted providers from gkdev (#3901) #3922
Integrations support self hosted providers from gkdev (#3901) #3922
Conversation
ede3ded
to
f00ba5b
Compare
@eamodio, @axosoft-ramint, @d13, To sum up what is happening here: new type of integration "cloud-github-enterprise" that works as enterprise but gets authorization as cloud. |
f00ba5b
to
b15b070
Compare
b15b070
to
fb6f815
Compare
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.
Overall it looks good - though I did have a few questions.
await this.container.storage.deleteSecret(key); | ||
await this.authenticationService.removeConfigured({ | ||
integrationId: this.authProviderId, | ||
domain: isSelfHostedIntegrationId(this.authProviderId) ? sessionId : undefined, | ||
}); |
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.
Nit for another day, but it feels like these operations should be combined into the AuthService rather than in the base class.
@sergeibbb Please feel free to take over from here including @eamodio 's review comments on my "configured integrations" changes. |
15c7e7f
to
672297d
Compare
Description
#3901
Checklist
Fixes $XXX -
orCloses #XXX -
prefix to auto-close the issue that your PR addresses