-
Notifications
You must be signed in to change notification settings - Fork 1.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
Converts GitHub integration authentication to use gk.dev GLVSC-554 #3356
Conversation
bdd1fd0
to
b657c75
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.
A couple of suggestions on the base class and override strategies - mainly suggesting that we leverage abstract classes/fns.
src/plus/integrations/authentication/integrationAuthentication.ts
Outdated
Show resolved
Hide resolved
src/plus/integrations/authentication/integrationAuthentication.ts
Outdated
Show resolved
Hide resolved
src/plus/integrations/authentication/integrationAuthentication.ts
Outdated
Show resolved
Hide resolved
src/plus/integrations/authentication/integrationAuthentication.ts
Outdated
Show resolved
Hide resolved
src/plus/integrations/authentication/integrationAuthentication.ts
Outdated
Show resolved
Hide resolved
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.
Apologies for multiple reviews - one more important thing I forgot in the previous review regarding "connect to remote".
d37fa99
to
6e6f918
Compare
d78a8fb
to
f06921d
Compare
355cb5c
to
afcb2f4
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.
Currently, when testing with a GitHub repo on this PR branch, it shows that my remotes are not connected, and Launchpad also doesn't recognize my existing (VScode built-in) connected GitHub integration.
I did a bit of investigation and found out why - we covered existing built-in GitHub tokens in the createSession
flow, but not in the getSession
flow.
4dbbd89
to
d47538e
Compare
Hey @sergeibbb, while reviewing your PR, I'd suggest the following code changes: 👉 Fixes integration.connect throwing error for GitHub
You can also review and apply these suggestions locally on your machine. Learn more about GitKraken Code Suggest
Join your team on GitKraken to speed up PR review. |
@sergeibbb There are issues with I made some updates to address this - see my code suggestion above. |
326de11
to
a1cf15f
Compare
…nProvider class GLVSC-554
that tries to check for existing GitHub session before accessing an integration on GKDev
…provider GLVSC-554 access it through a getter
…t createSession differently GLVSC-554 but share the common logic, which is implemented in the base class, of managing the created session
…t of the base flow GLVSC-554 stops calling the `super.createSession()` from the child.
because we never want to create a new one.
sets the expiration period to 1 year from now.
…createSession()" GLVSC-554
Ensures that manageCloudIntegrations is always called before attempting integration.connect for GitHub
Delete only cloud ones on syncing
… GLVSC-554 Checks for the cloud integration connection fist, and then uses the vscode session as a fallback if the cloud one isn't there.
a1cf15f
to
1ae8f4e
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.
This is looking great. I've been able to successfully test:
- With local integration, without GK integration
- With GK integration, without local integration
- With local integration fallback, after signing out of GK integration
d6426ec
to
f04bd36
Compare
Description
Introduces a provider for GitHub integration that tries to check for existing GitHub session before accessing an integration on GKDev (GLVSC-554)
Checklist
Fixes $XXX -
orCloses #XXX -
prefix to auto-close the issue that your PR addresses