Conversation
For now, HTTP code hasn't been moved more than necessary, left in existing location to aid merging
… full ssh:// URLs Also corrected duplicated test which was intended to test non-bare SSH URLs I assume
…upport GIT_SSH, Plink & TortoisePlink
…th interleaved byte streams)
…P reference server
…, since they're general
…e-use contexts based directly on endpoint equality for safety
…e.name.lfsurl Previously only supported SSH if it was derived from the main git remote url. Although this is the most likely case when using SSH (co-located git repo and binaries), also allow a separate URL to be used with the native SSH path too if required.
lfs/ssh_test.go
Outdated
There was a problem hiding this comment.
Change this to "github.com/github/git-lfs/vendor/_nuts/github.com/technoweenie/assert" to get the test to pass.
There was a problem hiding this comment.
Oops, didn't spot that when I merged in the nut changes, cheers.
There was a problem hiding this comment.
This interface isn't really ready to be extracted. We don't intend to maintain support for the current upload/download API once the batch API is finalized. I'm thinking an eventual interface would look something like this.
type ApiContext interface {
Endpoint() Endpoint
Close() error
DownloadAll(objects []*ObjectResource)
UploadAll(objects []*ObjectResource)
}I think an ApiContext should be responsible for a single Git LFS operation. That includes a smudge filter that downloads a single object, a push command that's uploading 100 objects from the range of commits. I think that would remove the need to access cached ApiContext objects through GetApiContext(). I can't think of a reason you'd need to get an ApiContext for multiple endpoints, or multiple contexts for the same endpoint in a single Git LFS command.
There was a problem hiding this comment.
OK - I was just working with what was there, happy for it to be 100% batch oriented too. In theory yeah that would eliminate the need for cached contexts within the usage model of the command line, although it does no harm for them to be there really; the SSH transport will always need a discrete setup & tear-down step anyway, so whether it's potentially around multiple ops or just one is neither here nor there in terms of added complexity.
There was a problem hiding this comment.
FWIW I was developing this in parallel with the interface morphing (Upload disappeared & Check/Object/Batch variants appeared) and it was really no big deal to stay in step. I don't think you necessarily need to wait until your interface is 'final' before allowing it to be abstracted like this - although depends on how many 3rd party implementations you were expecting (I wasn't really expecting any).
There was a problem hiding this comment.
Honestly, I'd love to reject all other implementations and figure out some way to implement custom ones through commands. Until then, I think we can merge this and move forward with the API design incrementally. We're likely not accepting other implementations any time soon. Each client makes it that much harder to evolve and maintain.
I'd really like to get rid of GetApiContext if we don't need it. That can wait until everything is using the *All() batch methods though.
There was a problem hiding this comment.
Yeah the concept that the context can survive over multiple API calls is necessary until you can guarantee that only one API call will be made per command, unless you want a separate SSH connection to be fired up for every file. Even after the *All() batch methods are implemented though, I think it's nice to have that flexibility - to replace it would just be equivalent to a global context per API and I'm not sure that actually simplifies much; instead baking in an assumption.
|
Before merging, I'd like to get some assurance that you'll be around to support it. We're not planning on using it in production anywhere. I don't see this being a problem, but I just want to be clear :) I also still need to set this up locally and see the flow working, and make sure our existing servers all still work. After merging, here are some things I'd like to see:
Regarding a git config fallback: If I understand this, it falls back to the hybrid https/ssh authentication method if |
|
Yeah I'd love to figure out how to fully test over real SSH instead of having to use a pipe, that last little piece of non-automated testing bothers me. I couldn't figure out a way to fake it without still having a gap - a fake ssh server in go is possible but it still wouldn't be 100% real. But then again, still better so maybe I should have a crack at it. Before you merge I'd like to do a few more tests myself anyway since I had to finish it while travelling and didn't have my office servers available to test with. Also I don't have a live implementation git-lfs-ssh-authenticate to test with, assume GH's implementation is not available as source - I've signed up for the GitHub LFS early access queue which I guess is the best way to test this, maybe you can get that enabled for me? Config fallback to remember the preferred route sounds sensible, the only issue is dealing with transitory error conditions, e.g. someone tries to push while offline etc - you wouldn't want to bake in that failure as a preference. I guess that means only setting the option after a successful use of that path. About maintenance, I'm assigned to this area by Atlassian for at least the next few months and have a personal interest in it anyway so would probably be tinkering with it even if I wasn't. I can't make any irrefutable promises for the future though, real life etc :) I think it's useful even if marked 'experimental' with GH's line being simply that you don't support it - the refactoring is useful anyway. Or, if you're worried it will slow you down when you're adapting the API, at least now I know you're OK with the principles you could merge just the stuff that's immediately useful (SSH URLs, plink/Tortoise support) and I can maintain my SSH fork in parallel & see what the community thinks. The main issue with that is that the refactoring required moving some code around which becomes harder to maintain over time. That said, it was only about 10 days work so if I had to do it manually again I could do it more quickly anyway - it might just not be always in sync. I'm happy with whichever approach you want to take. |
|
Found a couple of auth prompting issues now I can test in real environments, hold off merging either way until I've nailed them down. |
|
I am seemingly able to stall it sometimes with large numbers of files when using |
|
OK, finally figured out why - my misunderstanding of how your goroutines worked. Needs a bit of a design change to accommodate & serialise to the SSH pipe(s), I'll resubmit once I've sorted it out. |
This PR makes git-lfs compatible with a pure SSH server implementation, rather than only authenticating then doing everything else over HTTP. This is particularly useful for people self-hosting who don't want the overhead of setting up & maintaining a web server.
Corresponding reference server implementation is at https://github.com/sinbad/git-lfs-ssh-serve
PR in its current state focusses on a pure SSH route (including upload/download), future extensions might include supporting a further hybrid setup where again no web server is required for any API calls (UploadCheck/DownloadCheck) but the Upload/Download can still be redirected to hypermedia links (most likely S3).
PR also adds support for a few more SSH URL forms including custom ports. The existing git-lfs-authenticate route is not affected.