Skip to content
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

Determine remote endpoint for Git network operations #9139

Merged
merged 25 commits into from
Feb 26, 2020

Conversation

niik
Copy link
Member

@niik niik commented Feb 19, 2020

This is another continuation of the proxy support started in #9126 and #9127.

Now that we're going to start looking up the proxy to use for Git network operations we're going to have to have an understanding of what the remote endpoint is gonna be for any given network operation.

Corporate proxies are often set up so that a script determines which proxy to use based on the url that the client wants to access (for https urls the script usually only gets the protocol and domain). See #9127 for details on that.

Unfortunately for us there's not a 1:1 correlation between git command and url. Take the simplest case of git clone URL for example. While the initial request will be to URL it's possible that the repository could contain submodules pointing to other hosts. It's also possible that the repository is set up to use LFS in which case there may be subsequent requests to a dedicated LFS server.

While there might be multiple endpoints accessed by a single Git call we only have the ability to provide Git one proxy url per protocol (http/https) so we're gonna have do do a best-effort guess at a reasonable host. We're also gonna assume that the vast majority of repositories these days do all of their communications over https.

It's also worth noting that the model we've used to provide Git with authentication details (username/password) has been based on the premise that the same credentials will work for any submodule and/or LFS instance accessed during the operation.

I've introduced a substitute method for envForAuthentication called envForRemoteOperation. This will now be the centralized location for determining the proxy url. The actual process of setting the required environment variables will come in a follow up pr.

Notes for the reviewer

This PR touches sensitive parts of the codebase such as fetch, push, and pull. Please keep a lookout for anything that looks off or alters the behavior of either of these operations in a way not described below.

Affected operations

deleteBranch

deleteBranch is also responsible for deleting the branch on the remote which means that it's essentially a push operation. Here I've had to introduce a new git operation to load the url of the branch remote. The risk of not being able to resolve the remote url is slim here but there's a fallback anyway

Expected altered behavior: One extra Git exec to lookup the remote url of the remote tracking branch

checkout and revert

While not a network operation in Git's eyes it's possible that the repository is set up to use LFS which might have to communicate with its server on checkout or revert. While we can't tell for certain what the LFS server url is (without shelling out to git lfs) it's a pretty safe bet that the account endpoint is a decent substitute given that that's what Git LFS will use to authenticate.

Expected altered behavior: None

clone

We use whatever url is provided as the remote url

Expected altered behavior: None

fetch and fetchRefSpec

We already had access to IRemote objects a few levels up in the call chain to the changes in here are just forwarding that information down rather than only using the name property of the IRemote

Expected altered behavior: None

push

This one got a bit complicated because I really didn't want to risk introducing any new behavior here. See the code for a long comment explaining the tradeoff.

Expected altered behavior: None

Tutorial repository creation

Simply a refactor to pass the known remote url to envForRemoteOperation

Expected altered behavior: None

@tierninho
Copy link
Contributor

This PR touches sensitive parts of the codebase such as fetch, push, and pull. Please keep a lookout for anything that looks off or alters the behavior of either of these operations in a way not described below.

This may be premature, but I spent some time combing over the areas outlined above to search for potential inconsistencies, however nothing was found. Fetch, push, and pull, checkout, revert, clone, and the tutorial were tested, however LFS and submodules were not at this point. I will continue testing and report back if any out of the ordinary is noticed.

@outofambit outofambit self-requested a review February 20, 2020 13:51
This caused a "Pulling [Object object]" progress text
@fred652

This comment has been minimized.

Copy link
Contributor

@outofambit outofambit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two minor comments, looks good! 📳

app/src/lib/git/environment.ts Show resolved Hide resolved
app/src/lib/git/branch.ts Show resolved Hide resolved
@outofambit outofambit merged commit 8fe5a1b into development Feb 26, 2020
@outofambit outofambit deleted the env-for-remote-op branch April 21, 2020 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants