-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Enhances remote provider connection flow #4411
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: main
Are you sure you want to change the base?
Conversation
8eccc89 to
613c7fa
Compare
613c7fa to
b1c0c73
Compare
|
@axosoft-ramint What do we do with this? Reject, accept, transform to different requirements? cc @eamodio |
src/git/models/repositoryShape.ts
Outdated
| remotePath?: string; | ||
| remoteName?: string; |
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 see remotePath being added here but I don't see where it is being used?
Maybe call it bestRemoteName since in the only two places we use this, it's the so-called "best remote" of the repo?
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.
Fixed
| { integrationIds: [provider.integration!.id], source: this.source }, | ||
| href=${createCommandLink<ConnectRemoteProviderCommandArgs>( | ||
| 'gitlens.connectRemoteProvider', | ||
| { remote: provider.remoteName!, repoPath: repo.path }, |
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.
Is this guaranteed to be the remote that the current branch is located on since this is where the link is located?
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 a good question. For picking the right integration the remote is unnecessary, because the only thing that matters is the integration type.
The problem here is setting the default remote when the integration is connected.
The old flow, that goes through the web site (ConnectCloudIntegrationsCommand), does not set the default remote when the integration is connected.
In the new flow the default remote is set to the value of the parameter that might be wrong.
So, I suggest to make the remote field optional and if it's not set, then do not set the default remote on connecting an integration. By doing that we:
- keep the old behavior (not setting the remote at all)
- won't fail with setting a wrong remote as default.
What do you think?
0fc9dfc to
510dd0b
Compare
510dd0b to
abd6f1b
Compare
abd6f1b to
d9655ad
Compare
|
|
||
| export interface ConnectRemoteProviderCommandArgs { | ||
| remote: string; | ||
| remote?: string; |
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.
It does feel a bit weird that we could call "ConnectRemoteProvider" as a command without a remote supplied, but that's probably just semantics at this point.
LGTM will verify once it's in, thanks for updating it!
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.
It does feel a bit weird that we could call "ConnectRemoteProvider" as a command without a remote supplied
Yes. It's called "connect provider" but connects an integration. The question is what if I have 2 remotes: one for GitHub and another one for Azure and both are disconnected. But at some point on the previous version with ConnectCloudIntegrationsCommand it should have been defined somehow too.
In ConnectRemoteProviderCommand it can be called without a defined remote and there is a code substitute a value for remote:
With old ConnectCloudIntegrationsCommand we already had a provider and took an integration-id from it:

But how that provider has been picked? I'm going to check out the origin of the provider value there and maybe it will be possible to take a remote for it in that place. Or, maybe the ConnectRemoteProviderCommand could accept provider argument when remote is unknown, because having provider is enough for getting its integration.
|
This one disappeared from my feed/radar somehow, but I noticed today my review was requested again on this, so I assigned myself just to make it more visible to me |
|
@axosoft-ramint I didn't requested the review recently. I kept it on me because I was review the optional |

Description
This is a follow-up on #4387
Originates from this conversation:
How it should work (without a web page, using existing connection):
enable-remotes.mp4
How it works (it passes user through a web page) and what this pull request changes:
Verification steps
switch to a branch, that has an upstream from

originon remotes: disconnect, if there is a default remote then "Unset as default"
open "Home", connect.
on "remotes" check that the
originis selected as defaultedit the branch, change its upstream to another remote

disconnect and "Unset the default" again
open "Home", connect
on "remotes" check that the
ortheris set as defaultChecklist
Fixes $XXX -orCloses #XXX -prefix to auto-close the issue that your PR addresses