API Client and Lock Service#1236
Conversation
|
Hmm... looks like there was some weirdness going on with one of |
|
Argh, we have to rewrite all the imports. |
|
Try this: diff --git a/vendor/_nuts/github.com/xeipuuv/gojsonreference/reference.go b/vendor/_nuts/github.com/xeipuuv/gojsonreference/reference.go
index d4d2eca..3713a81 100644
--- a/vendor/_nuts/github.com/xeipuuv/gojsonreference/reference.go
+++ b/vendor/_nuts/github.com/xeipuuv/gojsonreference/reference.go
@@ -27,7 +27,7 @@ package gojsonreference
import (
"errors"
- "github.com/xeipuuv/gojsonpointer"
+ "github.com/github/git-lfs/vendor/_nuts/github.com/xeipuuv/gojsonpointer"
"net/url"
"path/filepath"
"runtime"
diff --git a/vendor/_nuts/github.com/xeipuuv/gojsonschema/jsonLoader.go b/vendor/_nuts/github.com/xeipuuv/gojsonschema/jsonLoader.go
index 0b40574..53b3733 100644
--- a/vendor/_nuts/github.com/xeipuuv/gojsonschema/jsonLoader.go
+++ b/vendor/_nuts/github.com/xeipuuv/gojsonschema/jsonLoader.go
@@ -38,7 +38,7 @@ import (
"runtime"
"strings"
- "github.com/xeipuuv/gojsonreference"
+ "github.com/github/git-lfs/vendor/_nuts/github.com/xeipuuv/gojsonreference"
)
var osFS = osFileSystem(os.Open)
diff --git a/vendor/_nuts/github.com/xeipuuv/gojsonschema/schema.go b/vendor/_nuts/github.com/xeipuuv/gojsonschema/schema.go
index 577c786..c4cb9f4 100644
--- a/vendor/_nuts/github.com/xeipuuv/gojsonschema/schema.go
+++ b/vendor/_nuts/github.com/xeipuuv/gojsonschema/schema.go
@@ -32,7 +32,7 @@ import (
"reflect"
"regexp"
- "github.com/xeipuuv/gojsonreference"
+ "github.com/github/git-lfs/vendor/_nuts/github.com/xeipuuv/gojsonreference"
)
var (
diff --git a/vendor/_nuts/github.com/xeipuuv/gojsonschema/schemaPool.go b/vendor/_nuts/github.com/xeipuuv/gojsonschema/schemaPool.go
index 70ae731..ab14242 100644
--- a/vendor/_nuts/github.com/xeipuuv/gojsonschema/schemaPool.go
+++ b/vendor/_nuts/github.com/xeipuuv/gojsonschema/schemaPool.go
@@ -30,7 +30,7 @@ import (
"errors"
"net/http"
- "github.com/xeipuuv/gojsonreference"
+ "github.com/github/git-lfs/vendor/_nuts/github.com/xeipuuv/gojsonreference"
)
type schemaPoolDocument struct {
diff --git a/vendor/_nuts/github.com/xeipuuv/gojsonschema/subSchema.go b/vendor/_nuts/github.com/xeipuuv/gojsonschema/subSchema.go
index b249b7e..a2e72a5 100644
--- a/vendor/_nuts/github.com/xeipuuv/gojsonschema/subSchema.go
+++ b/vendor/_nuts/github.com/xeipuuv/gojsonschema/subSchema.go
@@ -31,7 +31,7 @@ import (
"regexp"
"strings"
- "github.com/xeipuuv/gojsonreference"
+ "github.com/github/git-lfs/vendor/_nuts/github.com/xeipuuv/gojsonreference"
)
const ( |
|
@technoweenie thanks! Applied your commit, but it looks like we may still be missing some of |
|
On deps, I'd say this is a good time to move to go's 1.6's standard As for vendoring all sub-deps, this is a given if you expect the project to build straight after clone. The alternative is to not commit any of the vendored packages and require a post-clone command instead (e.g. Personally I prefer committing all deps because then the only time you have to worry about your package management is when you personally change the dependencies. But either works. |
api/lifecycle.go
Outdated
There was a problem hiding this comment.
I thought part of the idea of abstracting this was to make the interface non-specific to HTTP, but you're including http.Request in the interface here. Response conversely is non-protocol specific, which is more what I expected.
There was a problem hiding this comment.
You're right, but I had mentioned to @technoweenie that I thought this was a good intermediate step between the old HTTP-specific code and the new API client stuff. Eventually, I'd like to find the correct abstraction between the *http.Request and api.Request, but until we know what the SSH API is going to look like, this is difficult.
If it's okay with you, I think we should leave this and the Method field on RequestSchema as is until we can see what an SSH api would need specifically.
There was a problem hiding this comment.
Fair enough. Maybe a TODO comment though?
Yup, until then, I'd just update the imports here.
Nah, I think I'll try glide instead. |
|
OK, pushed a commit which uses the |
- rename client to v1, v2_client to client
The initial thought here is to introduce a MethodTestCase type that encapsulates the behavior of testing a single method in a particular given service. To do so, a httptest.Server is created and the schema is turned into a request which is fired at that server. Thet MethodTestCase, of course, knows how to respond to different requests, and the behavior of those responses is tested. What I dislike is that we have to write three things which are mostly the same to test any endpoint in any case on the API: 1) a request type (Go struct) 2) an expected response type (Go type) 3) the actual response (a mutltline Go string, which is really just JSON) This seems redundant, so I may explore other options for implementing this sort of thing in the future.
One of the things I'd mentioned that I disliked about the initial implementation of MethodTestCase was that it required you to type out three things (see previous commit). Of particular importance are items (2) and (3), the expected and actual response, one a Go type, the other a JSON string. This commit reduces that typing by 1/2. Since all we were doing was typing the same data in two different formats, the only thing that we were really testing was the encoding/json package. That is redundant, and a bad boundary to include during testing. Therefore, instead of having to type out the JSON-encoded string, this commit delegates that responsibility into the MethodTestCase's server, and just uses a `json.Encoder` to do the work. Sweet!
MethodTestCase had lots of problems, most caused by a good degree of ambiguity in what we were actually testing. AssertSchema(), on the other hand, simplifies this problem greatly by correctly identifying the responsibilities of an *api.RequestSchema. An *api.RequestSchema is responsible for one thing, and one thing only: to provide a template for requests to be made against the API, completely regardless of protocol or implementation. Most notably, these responsibilities do not include: - Preforming an HTTP request, or - reading the response of an http request So MethodTestCase gets simplified to a simple assert.Equal(). We expect to get some *api.RequestSchema out, and we expect it to have certain parameters. Now, all of the API's Service tests are rather simple: create a schema, compare, rinse and repeat. The only thing left to do is test the schema of the request and response payload. This is what I had tried to do with the original MethodTestCase implementation, but the way the system is desinged, trying to accomplish this involves integrating with too many unrelated parts. To address this, I plan to introduce a JSON Schema implementation that compares generated request/reponse payloads with their ideal types.
d934947 to
3c3ad2b
Compare
|
Huh, not sure why Travis ran the last test against Go 1.3.1 (not in the |
|
Looks good 🤘 |
|
Thanks @technoweenie, @sinbad! 👋 |
This PR introduces the Git LFS API client, as well as the
LockService, for interacting with the new LFS file locking API.API Client
The API client is broken down into several key components, all of which are described here:
Request Schemas
A request schema provides a definition or template type from which to build an actual request. Right now, the definition of this type is as follows:
Request/Response Lifecycle
The lifecycle type is responsible for a few things:
Right now, things are a little
net/httpspecific, and the only Lifecycle implementation is the*api.HttpLifecycletype. That type knows how to do all of the things described above in an HTTP-specific context. Internally, it uses the*http.Clienttype to execute the requests.In the future, I would like to abstract away the HTTP-specific stuff, and open the API up to an SSH, or even WebSocket implementation.
Services
Services are responsible for implementing API methods by providing code to generate
*api.RequestSchemas, which can be passed to the*api.Clientin use to be sent through the lifecycle subsystem, and eventually, a response is returned.Lock Service
This PR includes the
LockServicetype, which implements all of the APIs type definitions, as well as the three methods specified in the File Locking API proposal (Lock, Unlock, and Search).An example use of the
LockServicewould be as follows.schemapackageWe also introduced a way to test that our implementation of the API as defined in the
docs/directory is correct, by using JSON schema.This is a nice boundary to pick, since we have tests for the Service methods, and tests for the API client code, so the actual API implementation is tested separately.
To test a new API type, simply add a
*_schema.jsonfile to theapi/schema/directory, and you're good to go:or, alternatively:
Thanks in advance to anyone who reads through this PR, I appreciate you taking the time to look through this rather lengthy diff. 😄
Special thanks to @technoweenie and @sinbad for listening to my design proposals, as well as providing invaluable suggestions and advice along the way. 👋
Enjoy!