Allow using custom transfer agents directly #2429

aleb · 2017-07-20T14:15:37Z

Some custom transfer agents figure out everything by themselves and don't need
any authentication or transfer details from the API server.

Added the lfs.standalonetransferagent config option for specifying which
custom agent should be used directly.

aleb · 2017-07-20T14:16:00Z

If this is the right approach, I can add some tests.

The goal is to be able to use rsync through a custom transfer agent, see #379

technoweenie

Hey, this is clever. I suggested a different place to put the standalone custom transfer logic, but otherwise looks good 👍

For tests, I'd recommend looking at test/test-custom-transfers.sh and test/cmd/lfstest-customadapter.go for prior art.

technoweenie · 2017-07-20T15:47:35Z

tq/transfer_queue.go

-				q.wait.Done()
-			}
+	var bRes *BatchResponse
+	if q.manifest.standaloneTransferAgent != "" {


What do you think about moving this check into the Batch() function? That way standaloneTransferAgent just works for all tq.Batch() callers.

Indeed this could be moved in the Batch() function, but I think it might be better to get rid of that method altogether. All it does is to check for len(objects) which is already checked in tq.Batch(), and make a very simple call to tq.Batch .. and it's used in a single place only.

What other callers do you think there would be?

tq.Batch() is currently also used in test/git-lfs-test-server-api/main.go. I'm open to refactoring the tq package, but I think it should happen separately from this PR.

EDIT: Also, I agree that tq.Batch() is not necessary.

technoweenie · 2017-07-20T15:51:07Z

tq/transfer_queue.go

+		objects := make([]*Transfer, 0, len(batch))
+		for _, object := range batch {
+			objects = append(objects, &Transfer{
+				Name: object.Name,


Name isn't used in the custom transfer adapter protocol and can probably be omitted.

You are right, the name is not used in the protocol. Removed.

technoweenie · 2017-07-20T15:51:28Z

tq/transfer_queue.go

+				Name: object.Name,
+				Oid:  object.Oid,
+				Size: object.Size,
+				Path: object.Path,


I wonder if batch.ToTransfers() should just pass the Path along too, so it could be used here.

Looks much slimmer, but now the server receives also the Paths in the Batch API requests. I'll leave it like it is now.

aleb · 2017-07-27T15:15:42Z

I pushed an update, the test passes but not if I shut down the lfs server:

  # make sure git-lfs does not interact with the server when a standalone
  # transfer agent is set.
  curl -s "$(cat "$LFS_URL_FILE")/shutdown"

The error is this:

    17:08:06.236406 git.c:322               trace: built-in: git 'push' 'origin' 'master'
    17:08:06.237605 run-command.c:626       trace: run_command: 'git-remote-http' 'origin' 'http://127.0.0.1:61154/test-custom-transfer-standalone'
    fatal: unable to access 'http://127.0.0.1:61154/test-custom-transfer-standalone/': Failed to connect to 127.0.0.1 port 61154: Connection refused

Where should I look to find where it tries to connect to the server?

Some custom transfer agents figure out everything by themselves and don't need any authentication or transfer details from the API server. Added the `lfs.standalonetransferagent` config option for specifying which custom agent should be used directly.

aleb · 2017-07-31T14:40:39Z

This is mergeable IMO. Please have a look.

aleb · 2017-07-31T14:53:14Z

I figured that I should not stop the server used by the test. Clarified a bit the documentation about the setup/shutdown methods.

ttaylorr · 2017-07-31T21:51:19Z

@aleb this looks great, and thanks for your contribution to LFS! I've schedule this to be released in 2.3.0.

aleb · 2017-08-09T16:09:25Z

There is a minor problem when using a standalone custom-transfer agent. When pushing a branch to a remote, the git-lfs fails because it tries to connect to the inexistent remote server through HTTPS to check some locking thing. Before dying it suggests to run something like:

git config --replace-all lfs.https://SERVER/.locksverify false

This solves the problem, but I was wondering if the check should even be made when using a standalone custom-transfer agent. Does it makes sense to deactivate it when lfs.standalonetransferagent is set?

ttaylorr · 2017-08-09T20:52:31Z

Does it makes sense to deactivate it when lfs.standalonetransferagent is set?

I think it makes sense to deactivate this. The code you're looking for is here:

git-lfs/commands/uploader.go

Lines 124 to 127 in 76e82ac

    
           state := getVerifyStateFor(endpoint) 
        
           if state == verifyStateDisabled { 
        
           	return 
        
           }

PR-2429 has implemented a mechanism to unconditionally specify standalone custom transfer agents. This commit addresses two limitations, so that standalone custom transfer can be configured per remote. * Custom transfer agents may now be specified based on an URL prefix match on the API URL. * git-lfs passes the Git remote in the stage 1 initiation message. The standalone custom transfer agent can use it to determine the remote file location. One remote can, for example, point to a server that supports ssh login and use rsync. Another remote can point to GitHub. LFS works for both. Example Git config: ``` remote.github.url=... remote.origin.url=ssh://gitssh.example.com/git/some/repo lfs.customtransfer.rsync.path=git-lfs-rsync-agent lfs.https://gitssh.example.com/git/.standalonetransfer=rsync ``` The config assumes that `git-lfs-rsync-agent` determines the remote from the stage 1 init message and then inspecting `remote.origin.url` to infer the rsync host. <https://github.com/aleb/git-lfs-rsync-agent>, 2017-09-17, does not yet do that. [PR-2429] 09b7c53 'Allow using custom transfer agents directly', <git-lfs#2429>. CC: Alexandru Băluț <ab@daedalean.ai>

PR-2429 has implemented a mechanism to unconditionally specify standalone custom transfer agents. This commit addresses two limitations, so that standalone custom transfer can be configured per remote. * Custom transfer agents may now be specified based on an URL prefix match on the API URL. * git-lfs passes the Git remote in the stage 1 initiation message. The standalone custom transfer agent can use it to determine the remote file location. One remote can, for example, point to a server that supports ssh login and use rsync. Another remote can point to GitHub. LFS works for both. Example Git config: ``` remote.github.url=... remote.origin.url=ssh://gitssh.example.com/git/some/repo lfs.customtransfer.rsync.path=git-lfs-rsync-agent lfs.https://gitssh.example.com/git/.standalonetransfer=rsync ``` The config assumes that `git-lfs-rsync-agent` determines the remote from the stage 1 init message and then inspecting `remote.origin.url` to infer the rsync host. <https://github.com/aleb/git-lfs-rsync-agent>, 2017-09-17, does not yet do that. [PR-2429] 09b7c53 'Allow using custom transfer agents directly', <git-lfs#2429>. CC: Alexandru Băluț <ab@daedalean.ai> Signed-off-by: Steffen Prohaska <prohaska@zib.de>

PR-2429 has implemented a mechanism to unconditionally specify standalone custom transfer agents. This commit extends the mechanism to specify custom transfer agents based on an URL prefix match on the API URL. Together with the previous commit, which added the Git remote to the custom transfer stage 1 initiation message, standalone custom transfer can now be configured per remote. One remote can, for example, use Rsync transfer to an SSH server. Another remote can use standard LFS to GitHub. Example Git config: ``` remote.github.url=... remote.origin.url=ssh://gitssh.example.com/git/some/repo lfs.customtransfer.rsync.path=git-lfs-rsync-agent lfs.https://gitssh.example.com/git/.standalonetransferagent=rsync ``` The config assumes that `git-lfs-rsync-agent` determines the remote from the stage 1 init message and then inspecting `remote.origin.url` to infer the rsync host. <https://github.com/aleb/git-lfs-rsync-agent>, 2017-09-17, does not yet do that. [PR-2429] 09b7c53 'Allow using custom transfer agents directly', <git-lfs#2429>. CC: Alexandru Băluț <ab@daedalean.ai> Signed-off-by: Steffen Prohaska <prohaska@zib.de>

technoweenie suggested changes Jul 20, 2017

View reviewed changes

aleb force-pushed the master branch from 362f3c0 to 50c64ed Compare July 27, 2017 15:12

aleb force-pushed the master branch from 50c64ed to 881040d Compare July 27, 2017 15:16

aleb force-pushed the master branch from 881040d to 09b7c53 Compare July 31, 2017 14:32

ttaylorr approved these changes Jul 31, 2017

View reviewed changes

ttaylorr merged commit f0d355f into git-lfs:master Jul 31, 2017

ttaylorr added this to the v2.3.0 milestone Jul 31, 2017

aleb mentioned this pull request Aug 10, 2017

commands: Disable lock verification when using a standalone custom-tr… #2499

Merged

sprohaska mentioned this pull request Sep 17, 2017

Support standalone custom transfer based on API URL prefix match #2590

Merged

ttaylorr mentioned this pull request Aug 30, 2018

config safe keys don't allow lfs.customtransfer, lfs.standalonetransferagent, etc #3204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using custom transfer agents directly #2429

Allow using custom transfer agents directly #2429

aleb commented Jul 20, 2017

aleb commented Jul 20, 2017 •

edited

Loading

technoweenie left a comment

technoweenie Jul 20, 2017

aleb Jul 24, 2017 •

edited

Loading

technoweenie Jul 24, 2017 •

edited

Loading

technoweenie Jul 20, 2017

aleb Jul 24, 2017

technoweenie Jul 20, 2017

aleb Jul 24, 2017 •

edited

Loading

aleb commented Jul 27, 2017

aleb commented Jul 31, 2017

aleb commented Jul 31, 2017

ttaylorr commented Jul 31, 2017

aleb commented Aug 9, 2017

ttaylorr commented Aug 9, 2017

Allow using custom transfer agents directly #2429

Allow using custom transfer agents directly #2429

Conversation

aleb commented Jul 20, 2017

aleb commented Jul 20, 2017 • edited Loading

technoweenie left a comment

Choose a reason for hiding this comment

technoweenie Jul 20, 2017

Choose a reason for hiding this comment

aleb Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

technoweenie Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

technoweenie Jul 20, 2017

Choose a reason for hiding this comment

aleb Jul 24, 2017

Choose a reason for hiding this comment

technoweenie Jul 20, 2017

Choose a reason for hiding this comment

aleb Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

aleb commented Jul 27, 2017

aleb commented Jul 31, 2017

aleb commented Jul 31, 2017

ttaylorr commented Jul 31, 2017

aleb commented Aug 9, 2017

ttaylorr commented Aug 9, 2017

aleb commented Jul 20, 2017 •

edited

Loading

aleb Jul 24, 2017 •

edited

Loading

technoweenie Jul 24, 2017 •

edited

Loading

aleb Jul 24, 2017 •

edited

Loading