Fix #260: Add REST API client and use it in ipfs-cluster-ctl #263
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
Closed
This adds the pakage api/rest/client which implements a go-client for the REST API component. It also update the ipfs-cluster-ctl tool to rely on it. Originally, I wanted this to live it in it's own separate repository, but the api client uses /api/types.go, which is part of cluster. Therefore it would need to import all of cluster as a dependency. ipfs-cluster-ctl would also need to import go-ipfs-cluster-api-client as a dependency, creating circular gx deps which would be a mess to maintain. Only the splitting of cluster in multiple repositories (at least for api, rest, ipfs-cluster-ctl, rest/client and test) would allow better dependency management by allowing rest/client and the ctl tool to only import what is needed, but this is something which brings maintenance costs and can probably wait a bit until cluster is more stable. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
hsanjuan
force-pushed
the
feat/260-api-client
branch
from
3ad0cbd to
e0e3202
Compare
|
Had to fix some golints from locker PR in order for tests to pass and ended up un-exporting the Locker thing. |
ZenGround0
reviewed
Dec 6, 2017
| } | ||
| urlPrefix += host | ||
|
|
||
| if lvl := cfg.LogLevel; lvl != "" { |
There was a problem hiding this comment.
Should we have a default level in the case lvl == ""?
api/rest/client/client_test.go
Outdated
| t.Fatal("should be able to create a new Api: ", err) | ||
| } | ||
|
|
||
| // No keep alive! Otherwise tests hang with |
There was a problem hiding this comment.
Is this comment still valid? If not maybe we can remove it. If so, shouldn't we still disable keepalives?
api/rest/client/client_test.go
Outdated
| defer api.Shutdown() | ||
| c, _ = NewClient(&Config{}) | ||
| ids, err := c.Peers() | ||
| if err == nil { |
There was a problem hiding this comment.
Am I correct that this will not error if the machine running tests is also running ipfs-cluster-service on the default listening address? If so, is it ok for us to assume that cluster is not running externally in the tests?
* Set default logging facility * Remove old keep-alive comment in tests * Use a port for TestPeersWithErrors which is not default License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
ZenGround0
previously approved these changes
Dec 7, 2017
License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
This allows taking advantage of connection keep alive by having the api client re-use the same connection. Additionally, an option to close connections after every request is provided. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
As before, exit status 1 means a client application error and exit status 2 means a server-returned error. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
|
Glad that sharness helped catch a couple of issues :) |
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds the pakage api/rest/client which implements a go-client
for the REST API component. It also update the ipfs-cluster-ctl
tool to rely on it.
Originally, I wanted this to live it in it's own separate repository,
but the api client uses /api/types.go, which is part of cluster.
Therefore it would need to import all of cluster as a dependency.
ipfs-cluster-ctl would also need to import go-ipfs-cluster-api-client
as a dependency, creating circular gx deps which would be a mess to
maintain.
Only the splitting of cluster in multiple repositories (at least for
api, rest, ipfs-cluster-ctl, rest/client and test) would allow better
dependency management by allowing rest/client and the ctl tool
to only import what is needed, but this is something which brings
maintenance costs and can probably wait a bit until cluster is more stable.
License: MIT
Signed-off-by: Hector Sanjuan code@hector.link