Lay out some foundational works for the client #1
Conversation
There was a problem hiding this comment.
Good first cut of the foundational work.
I have a bunch of comments on various things. None of them necessarily need to be fixed in this PR, but we should open a bunch of issues for work items that fall out of the review.
I think some of the stuff here will change a little as the synsev3 doc gets finalized, but it shouldn't be anything major.
synse/config.go
Outdated
|
|
||
| // Options is the root config options. | ||
| type Options struct { | ||
| Server ServerOptions |
There was a problem hiding this comment.
I'm wondering if we need ServerOptions to be a separate struct. I wanna get your thoughts on this, but it seems like whenever someone will be constructing an Options struct, they will need to specify the server options as well. In that case, we could just make those fields part of Options, e.g.
type Options struct {
Address string
Timeout time.Duration
Retry RetryOptions
}There was a problem hiding this comment.
The reason I used ServerOptions was because:
- I wanted to have some kind of structure for the root config. I thought that Address and Timeout was somewhat related and could belong to a “server” group.
- I was thinking about other server-related config options that could be added in the future once v3 proposal was set, such as transport protocol (http, websocket), security related stuff (tls, role access,…), and so on.
However, now that I think about this again, I find that ServerOptions is not really necessary. These options could totally stand alone by themselves in separate structs. That said, I will get rid of the ServerOptions in this PR. More configurable options will be discussed in #3.
There was a problem hiding this comment.
👍
FWIW, ServerOptions was not a bad thought and I initially probably would have done the same thing. It was only when I went back through and looked at the example usage that I thought we could simplify. From a flexibility standpoint, the nesting makes sense, but from a usability standpoint we want to make configuring it as simple as we can.
synse/http.go
Outdated
| c = c.SetHostURL(fmt.Sprintf("http://%s/synse/", opt.Server.Address)) | ||
|
|
||
| if opt.Server.Timeout == 0 { | ||
| // FIXME - find a better way to use default options here? |
There was a problem hiding this comment.
I've used this before: https://github.com/creasty/defaults
It works well and is pretty lightweight. It may be overkill if our only default right now is for timeout, but its likely we will add other configs in the future which could leverage it.
| return errors.Wrap(err, "failed to cache api version") | ||
| } | ||
|
|
||
| versionedPath := fmt.Sprintf("%s/%s", c.apiVersion, path) |
There was a problem hiding this comment.
We'll probably want to separate out the URL construction from the request making at some point, otherwise there will be duplicate code when it comes to POSTing data via write.
synse/http.go
Outdated
| } | ||
|
|
||
| versionedPath := fmt.Sprintf("%s/%s", c.apiVersion, path) | ||
| err = c.getUnversioned(versionedPath, okResp, errResp) |
There was a problem hiding this comment.
Another thought: we could have different constructors for clients, e.g. (pseudocode)
// Creates a new synse client, does not specify API version.
func NewSynseClient() (*Client, error)
// Creates a new synse client with API v3
func NewSynseV3Client() (*Client, error)Then, when it comes to URL construction we have some options:
- if the client has a version specified (e.g. "v3", use it)
- if the client does not have a version specified, look it up using the
/versionendpoint.
This is ultimately a larger bit of work as it relates to backwards compatibility, so it doesn't need to be addressed now, but we should definitely open an issue for it.
| Context string `json:"context"` | ||
| } | ||
|
|
||
| // Status describes a response for `/test` endpoint. |
There was a problem hiding this comment.
can open an issue - once we have synse v3 actually in place, we can update the documentation to include a link to the spec
| // synse.go provides a client API for Synse Server. | ||
|
|
||
| // Client API for Synse Server. | ||
| type Client interface { |
synse/version.go
Outdated
| @@ -0,0 +1,49 @@ | |||
| package synse | |||
|
|
|||
| // version.go defines version information of the client. | |||
There was a problem hiding this comment.
We shouldn't need this file since this is just a client library, not a standalone binary. If we're not building anything, there will be no build-time information to use here.
There was a problem hiding this comment.
I am not totally clear on why a client library like this should not need versioning. I was thinking about the client library that we used recently, namely kubernetes/client-go, and thought we could have such similar versioning so we would know what version to point to. For example, as in blackbox, we used v8.0.0. k8s client.
There was a problem hiding this comment.
We'll have versioning for sure, but that will happen at a higher level using things like github tags. This pattern is used for things that produce binaries (e.g. like CLI, blackbox, etc) that run a program and provides a way to expose that version information at runtime (note that it only exposes it, it doesn't actually define the version). Since this will just be a library that other Go projects will use (e.g. this won't have its own binary), we can't really use this to expose the version in a meaningful way anyways.
For this project, using GitHub tags should be enough to designate a version for the library. Then when a different project uses it, we can vendor it to that version, e.g.
[[constraint]]
name = "github.com/vapor-ware/synse-client-go"
version = "1.0.0"
The works in this PR include:
/version,/test,/configroutes.dep,setup,test,cover.