Feat #305: Libp2p support for REST API #349

hsanjuan · 2018-03-14T23:00:58Z

This does something very cool:

Since the cluster peer has a libp2p host, we can pipe the http traffic under encrypted libp2p streams and basically get encrypted traffic to/from the API for free, without needing to setup certificates.

Doing this has required a bunch of changes:

First we need to create the Host outside the Cluster component, so we can re-use it for the API component.
Then we need to have the API component take a host.
We also support that the API component creates a fully different host (with different key etc). But this is not default. But if the user provides ID, PrivateKey, ListenAddress we do that
Then if we want to use libp2p we use a gostream listener for libp2p-http protocol and use it with the standard http.Server
Then the rest client needs to be able to talk to this, rather than to http endpoint, so we create a host and use go-libp2p-http to register a custom transport and create a libp2p-powered http.Client with it
Then we need to have ipfs-cluster-ctl figure out when the --host parameter is an libp2p multiaddress (<...>/ipfs/peer_id), and configure the client accordingly.

The rest: expand the tests to use both plain and tunneled http, deal with PNet, use new libp2p host constructors, use new Pnet helpers and other cosmetics.

This is a very big PR, but the commits in correspond to each step, are self-contained to components and the log messages explain the change, so probably easy to check one by one.

Note, there will still be some refactoring/function splitting coming (I want to leave a green codeclimate).

Fixes #305 .

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

NewCluster() now takes an optional Host parameter. The rationale is to allow to re-use an existing libp2p Host when creating the cluster. The NewClusterHost method now allows to create a host with the options used by cluster. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

This adds support for parameters to create a libp2p host in the REST API configuration: ID, PrivateKey and ListenMultiaddr. These parameters default to nil/empty and are ommited in the default configuration. They are only supposed to be used when the user wants the REST API to use a different libp2p host than a provided one (upcoming changes). Pnet protector not supported yet in this case. Underlying basic auth should cover that front. Will implement if someone has a usecase. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

This commits allows restapi to serve/tunnel http on a libp2p stream. NewWitHost(...) allows to provide a libp2p host during initialization which is then used to obtain a listener with go-libp2p-gostream. Alternatively, if the configuration provides an ID, PrivateKey and Libp2pListenAddr, a host is created directly by us and used to get the listener. The protocol tag used is provided by the p2phttp library which will be used by the client. All tests now run against the libp2p node too. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

This adds support for libp2p-tunneled http to the rest api component. If PeerAddr is specified in the configuration, then we will create a libp2p host and communicate with the API using that. Tests run now in both http and libp2p mode. Note: pnet support not included, but coming up License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Also, re-use some convinient functions provided by pnet. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

…that case. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

coveralls · 2018-03-15T16:47:51Z

Coverage decreased (-0.2%) to 67.514% when pulling 8e7ca5a on feat/restapi-libp2p into 1abeff6 on master.

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

So they can serve as multi-module helpers without having circular deps. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Should reduce complexity (codeclimate). License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

lanzafame

This is awesome @hsanjuan. A few minor nitpicks and questions. Otherwise, LGTM.

lanzafame · 2018-03-20T00:33:27Z

api/rest/client/client.go

+func (c *Client) setupHostname() error {
+	// When no host/port/multiaddress defined, we set the default
+	if c.config.APIAddr == nil && c.config.Host == "" && c.config.Port == "" {
+		c.config.APIAddr, _ = ma.NewMultiaddr(DefaultAPIAddr)


Why not return the error from NewMultiaddr?

not feeling super guilty to panic when anyone sets DefaultAPIAddr to something stupid, but ok :)

lanzafame · 2018-03-20T00:39:11Z

api/rest/client/client.go

+		resolveCtx, cancel := context.WithTimeout(c.ctx, c.config.Timeout)
+		defer cancel()
+		resolved, err := madns.Resolve(resolveCtx, c.config.APIAddr)
+		c.config.APIAddr = resolved[0]


Missing error check for madns.Resolve? If madns.Resolve fails, the slice access will panic.

lanzafame · 2018-03-20T00:48:08Z

api/rest/client/request.go

@@ -19,7 +19,7 @@ func (c *Client) do(method, path string, body io.Reader, obj interface{}) error
 }

 func (c *Client) doRequest(method, path string, body io.Reader) (*http.Response, error) {
-	urlpath := c.urlPrefix + "/" + strings.TrimPrefix(path, "/")
+	urlpath := c.net + "://" + c.hostname + "/" + strings.TrimPrefix(path, "/")


This could be a good use case for url.URL. @hsanjuan Thoughts?

I've considered it. It doesn't add much (other than lines of code). I'll keep it in mind if we ever need to escape query params in more than one place.

lanzafame · 2018-03-20T00:51:52Z

api/rest/client/transports.go

+		return err
+	}
+
+	// This should resolve addr too.


To clarify, is this a TODO comment or h.Peerstore().AddAddr() resolves the address when adding it?

Good catch. I thought it did, but it doesn't.

lanzafame · 2018-03-20T01:10:34Z

api/rest/config.go

@@ -97,23 +122,19 @@ func (cfg *Config) Default() error {
 // Validate makes sure that all fields in this Config have
 // working values, at least in appearance.
 func (cfg *Config) Validate() error {
-	if cfg.ListenAddr == nil {


All these if statements can be placed in a switch statement that has no expression, i.e.

switch { case cfg.ReadTimeout < 0: return errors.New("restapi.read_timeout is invalid") case cfg.ReadHeaderTimeout < 0: return errors.New("restapi.read_header_timeout is invalid") <etc> }

See go wiki.

yeah, and look better

lanzafame · 2018-03-20T02:44:00Z

api/rest/config.go

@@ -202,13 +300,28 @@ func (cfg *Config) ToJSON() (raw []byte, err error) {
 	}()

 	jcfg := &jsonConfig{}


Is there any reason to not move the field initialisations inside the struct initialisation?

jcfg := &jsonConfig{ HTTPListenMultiaddress: cfg.HTTPListenAddr.String(), <etc> }

marginally clearer to me when not all the fields in the object are initialized within the constructor. But ok.

lanzafame · 2018-03-20T03:52:19Z

api/rest/restapi.go

+		return nil
+	}
+
+	// Make new host. Override existing


Why are we providing a host.Host to NewAPIWithHost if it gets overridden regardless?

The API component is passed in the main cluster component's host (see 488 in ipfs-cluster-service/main.go). If the api config doesn't specify an independent address for a new host to listen on then the api will reuse the same host. This was a bit hard to follow and though there is some documentation in the comment on line 198, perhaps we could add this to the docs somewhere. In particular would be good to remind users that even if they don't specify a libp2p listen addr in their api config they will still have libp2p listening enabled by default. Perhaps libp2p listening on the default host should be turned off or on by a flag.

As discussed, only gets overriden when there is configuration for a custom host. So configuration overrides provided.

@ZenGround0 it doesn't hurt to have libp2p listening enabled by default with the cluster host. Notice that the custom-host options are hidden by default in default configs, meaning, I only want expert users (or those who really read the godocs) to use custom-host. Until we revamp the config, I think this is going to be more or less undocumented.

lanzafame · 2018-03-20T04:07:14Z

api/util.go

+// encapsulates a new /ipfs/<peerID> address.
+func Libp2pMultiaddrJoin(addr ma.Multiaddr, p peer.ID) ma.Multiaddr {
+	pidAddr, err := ma.NewMultiaddr("/ipfs/" + peer.IDB58Encode(p))
+	// let this break badly


Maybe rename function to MustLibp2pMultiaddrJoin to convey that it will panic if it fails?

lanzafame · 2018-03-20T04:45:14Z

config/util.go

+	Name string
+}
+
+// ParseDurations takes a time.Duration src and saves it to the given dst. into the given


Remove leftover comment on the end of the line, into the given.

lanzafame · 2018-03-20T04:56:08Z

ipfs-cluster-ctl/main.go

-configured with the --host option.
+configured with the --host option. To use the secure libp2p-http
+API endpoint, use "--host" with the full cluster libp2p listener
+address (including the "/ipfs/<peerID>" part), and --secret.


It could be a bit clearer that --secret is referring to the CLUSTER_SECRET.

It's in the help for the flag, but added explanation.

ZenGround0

Still working through this, here are my comments halfway through commit 6.

ZenGround0 · 2018-03-19T14:33:49Z

package.json

@@ -56,9 +56,9 @@
      "version": "2.3.6"
    },
    {
-      "author": "whyrusleeping",
+      "author": "hsanjuan",


Is this repo published somewhere now? I looked under your repos as you are the new author but couldn't find it.

https://github.com/gxed/go4-lock

I'm the packager, that's why my name is there. Honestly, it's difficult to figure out the true source of something with gx.

ZenGround0 · 2018-03-19T14:37:13Z

cluster.go

-		return nil, err
+
+	if host == nil {
+		h, err := NewClusterHost(ctx, cfg)


Is there something wrong with host, err = NewClusterHost(ctx, cfg)? It's a small change but would reduce noise a little bit if this is ok.

ZenGround0 · 2018-03-19T14:38:30Z

cluster.go

@@ -1097,71 +1095,6 @@ func (c *Cluster) Peers() []api.ID {
 	return peers
 }

-// makeHost makes a libp2p-host.


Great to see this going away

ZenGround0 · 2018-03-19T14:47:28Z

clusterhost.go

+	ma "github.com/multiformats/go-multiaddr"
+)
+
+// NewClusterHost creates a libp2p Host with the options in the from the


typo: "in the from the"

ZenGround0 · 2018-03-19T14:53:26Z

clusterhost.go

+}
+
+// EncodeProtectorKey converts a byte slice to its hex string representation.
+func EncodeProtectorKey(secretBytes []byte) string {


As this is simply a wrapper around hex.EncodeToString I am uncertain that a separate function is warranted for this.

ZenGround0 · 2018-03-20T16:46:36Z

api/rest/restapi.go

+}
+
+func (api *API) setupLibp2p(ctx context.Context) error {
+	if api.config.Libp2pListenAddr == nil && api.host == nil {


this check for api.host seems gratuitous as the same check happens on line 175 and the code between here and there can execute without problem regardless.

ZenGround0 · 2018-03-20T16:55:20Z

api/rest/restapi.go

+		return nil
+	}
+
+	// Make new host. Override existing


The API component is passed in the main cluster component's host (see 488 in ipfs-cluster-service/main.go). If the api config doesn't specify an independent address for a new host to listen on then the api will reuse the same host. This was a bit hard to follow and though there is some documentation in the comment on line 198, perhaps we could add this to the docs somewhere. In particular would be good to remind users that even if they don't specify a libp2p listen addr in their api config they will still have libp2p listening enabled by default. Perhaps libp2p listening on the default host should be turned off or on by a flag.

ZenGround0 · 2018-03-20T16:56:51Z

api/rest/restapi.go

 func NewAPI(cfg *Config) (*API, error) {
-	err := cfg.Validate()


This doesn't get called anymore when creating a new api component and I'm guessing it still should.

you' re right!

ZenGround0 · 2018-03-20T17:13:24Z

api/rest/restapi.go

+			for _, a := range api.host.Addrs() {
+				logger.Infof("  - %s/ipfs/%s", a, api.host.ID().Pretty())
+			}
+			// TODO(h): Do we need a different server?


Was this resolved?

ZenGround0 · 2018-03-20T17:57:48Z

api/rest/restapi.go

@@ -317,6 +434,9 @@ func (api *API) Shutdown() error {
 // requests.
 func (api *API) SetClient(c *rpc.Client) {
 	api.rpcClient = c
+
+	// One notification for http server and one for libp2p server.
+	api.rpcReady <- struct{}{}


I think this will block indefinitely in the case where the api starts up with only an http server or only a libp2p server

no, I increased channel capacity to two elements.

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan · 2018-03-20T18:54:16Z

all comments addressed

lanzafame

Just a comment typo.

lanzafame · 2018-03-20T23:18:38Z

api/rest/restapi.go

-
-	// Make new host. Override existing
+	// Make new host. Override any provided existing one
+	// iif we have config for a custom one,


typo: iif and stray , at the end

https://en.wikipedia.org/wiki/If_and_only_if

doesn' t make much sense anyways

iff is fine, iif is not 👍

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

ZenGround0

More comments

ZenGround0 · 2018-03-20T20:18:07Z

api/rest/restapi_test.go

 	apiMAddr, _ := ma.NewMultiaddr("/ip4/127.0.0.1/tcp/0")
+	h, err := libp2p.New(context.Background(), libp2p.ListenAddrs(apiMAddr))


Is it ok for libp2p and http to listen on the same network address?

Port 0 means it will get assigned a random port so, not the same address in the end.

ZenGround0 · 2018-03-20T20:36:41Z

api/rest/restapi_test.go

 	httpResp, err := c.Do(req)
 	processResp(t, httpResp, err, resp)
 }

+type testF func(t *testing.T, url urlF)
+
+func testBothEndpoints(t *testing.T, test testF) {


This function makes me really happy

ZenGround0 · 2018-03-20T20:59:30Z

api/rest/restapi_test.go

 }

 func TestAPIPeerRemoveEndpoint(t *testing.T) {
 	rest := testAPI(t)
 	defer rest.Shutdown()

-	makeDelete(t, apiURL(rest)+"/peers/"+test.TestPeerID1.Pretty(), &struct{}{})
+	tf := func(t *testing.T, url urlF) {
+		makeDelete(t, rest, url(rest)+"/peers/"+test.TestPeerID1.Pretty(), &struct{}{})


I'm wondering if there is a better way for the test to ensure that the mock endpoint corresponding to Delete on /peers was called as a result of making this request. Right now I don't know if anything will visibly fail if this doesn't happen. I realize this doesn't pertain to this PR but the previous test code and so maybe a fix doesn't belong here.

If hitting an unexistent endpoint (or it returns an error) processResp will fail, I think

ZenGround0 · 2018-03-21T02:05:22Z

api/rest/client/client.go

-	if cfg.Timeout == 0 {
-		cfg.Timeout = DefaultTimeout
+	c.client = &http.Client{
+		Transport: tr,


Why not set c.transport to tr? What else would the transport field be used for.

ZenGround0 · 2018-03-21T02:56:10Z

api/rest/client/methods_test.go

 )

+func testClients(t *testing.T, api *rest.API, f func(*testing.T, *Client)) {


why don't these run in parallel?

if that makes you happy :)

ZenGround0 · 2018-03-21T03:39:14Z

ipfs-cluster-ctl/main.go

@@ -81,7 +82,12 @@ func main() {
 		cli.StringFlag{
 			Name:  "host, l",
 			Value: defaultHost,
-			Usage: "multiaddress of the IPFS Cluster service API",
+			Usage: "multiaddress of the IPFS Cluster REST API HTTP or LibP2P endpoint",


Right now this is a little misleading because both libp2p + default endpoints serve HTTP.

Any ideas? I'm not sure how to explain this in a 120 char line.. (it's better explained above).

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

ZenGround0

One more small thing and then I am happy with this.

ZenGround0 · 2018-03-21T13:16:25Z

api/rest/config.go

@@ -209,23 +213,9 @@ func (cfg *Config) loadHTTPOptions(jcfg *jsonConfig) error {
 	key := jcfg.SSLKeyFile
 	cfg.pathSSLCertFile = cert
 	cfg.pathSSLKeyFile = key


looks like these lines can be deleted as cfg.tlsOptions() sets cfg.pathSSLCertFile and cfg.pathSSLKeyFile

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

ZenGround0

LGTM!

lanzafame

LGTM

Fix go4-lock name in package.json

836d552

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan self-assigned this Mar 14, 2018

ghost added the status/in-progress In progress label Mar 14, 2018

hsanjuan added 4 commits March 15, 2018 00:04

consensus/raft: use new host constructor in tests

07a6685

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Create cluster Host in ipfs-cluster-service.

5956dce

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan force-pushed the feat/restapi-libp2p branch from df8efb3 to 822402f Compare March 14, 2018 23:05

hsanjuan mentioned this pull request Mar 14, 2018

REST API and ipfs-cluster-ctl should be able to talk over libp2p #305

Closed

hsanjuan force-pushed the feat/restapi-libp2p branch from 822402f to b5d2bb8 Compare March 15, 2018 13:46

hsanjuan added 5 commits March 15, 2018 17:14

api/rest/client: add private networks support.

4bed9d0

Also, re-use some convinient functions provided by pnet. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

cluster-service: re-use cluster libp2p host for rest API by default

3eee5d0

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

ipfs-cluster-ctl: Allow libp2p multiaddress as --host. Use libp2p in …

73581b0

…that case. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan force-pushed the feat/restapi-libp2p branch from b5d2bb8 to 73581b0 Compare March 15, 2018 16:16

hsanjuan changed the title ~~WIP: Feat/restapi libp2p~~ Feat #305: Libp2p support for REST API Mar 15, 2018

hsanjuan requested a review from ZenGround0 March 15, 2018 16:34

hsanjuan added need/review Needs a review and removed status/in-progress In progress labels Mar 15, 2018

hsanjuan added this to the Open issues with difficulty:medium or lower milestone Mar 15, 2018

Feat #305: Golinting

f74c7b2

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

ghost added the status/in-progress In progress label Mar 16, 2018

hsanjuan added 6 commits March 16, 2018 13:23

rest/client: refactor transport setup (codeclimate)

2227e5e

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Relocate multiaddrJoin and multiaddrSplit to api/types.h

a73d7e6

So they can serve as multi-module helpers without having circular deps. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

api: move some util functions to util.go (codeclimate file lines)

5cd048a

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

api/rest: refactor LoadJSON (codeclimate)

e5ac535

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

api/rest: refactor run() (codeclimate)

b989e17

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Config: provide utility to parse duration arguments. Use it.

a9a58a5

Should reduce complexity (codeclimate). License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan added 2 commits March 16, 2018 14:55

api/rest: refactorings in config.go (codeclimate)

02bde26

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

ipfs-cluster-ctl: fix sharness

23d2db1

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

lanzafame suggested changes Mar 20, 2018

View reviewed changes

hsanjuan mentioned this pull request Mar 20, 2018

Feat #339: faster tests #350

Merged

ZenGround0 requested changes Mar 20, 2018

View reviewed changes

hsanjuan added 2 commits March 20, 2018 19:35

rest/libp2p-http: address lanzafame's review

09f4c9f

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

rest/libp2p-http: address @ZenGround0 comments

6777122

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

lanzafame suggested changes Mar 20, 2018

View reviewed changes

Use iff iff it is necessary

62bdbaa

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

ZenGround0 reviewed Mar 21, 2018

View reviewed changes

api/rest/client: run endpoint tests in parallel

4ea873c

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan requested review from ZenGround0 and lanzafame March 21, 2018 08:51

hsanjuan mentioned this pull request Mar 21, 2018

Fix #269: Add IPFS() method to api/rest/client #356

Merged

ZenGround0 reviewed Mar 21, 2018

View reviewed changes

rest/config: remove dup https options

8e7ca5a

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

ZenGround0 approved these changes Mar 21, 2018

View reviewed changes

lanzafame approved these changes Mar 26, 2018

View reviewed changes

hsanjuan merged commit a4adce6 into master Mar 26, 2018

hsanjuan deleted the feat/restapi-libp2p branch March 26, 2018 12:22

ghost removed the status/in-progress In progress label Mar 26, 2018

hsanjuan mentioned this pull request Mar 27, 2018

Release 0.3.5 #330

Closed

hsanjuan mentioned this pull request Apr 15, 2018

Rework README, ROADMAP.md docs #337

Closed

hsanjuan mentioned this pull request Jun 1, 2018

server gave HTTP response to HTTPS client #279

Closed

		@@ -202,13 +300,28 @@ func (cfg *Config) ToJSON() (raw []byte, err error) {
		}()

		jcfg := &jsonConfig{}

		func NewAPI(cfg Config) (API, error) {
		err := cfg.Validate()

		apiMAddr, _ := ma.NewMultiaddr("/ip4/127.0.0.1/tcp/0")
		h, err := libp2p.New(context.Background(), libp2p.ListenAddrs(apiMAddr))

		)

		func testClients(t testing.T, api rest.API, f func(testing.T, Client)) {

Feat #305: Libp2p support for REST API #349

Feat #305: Libp2p support for REST API #349

Conversation

hsanjuan commented Mar 14, 2018 • edited

coveralls commented Mar 15, 2018 • edited

lanzafame left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZenGround0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsanjuan commented Mar 20, 2018

lanzafame left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZenGround0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZenGround0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZenGround0 left a comment

Choose a reason for hiding this comment

lanzafame left a comment

Choose a reason for hiding this comment

hsanjuan commented Mar 14, 2018 •

edited

coveralls commented Mar 15, 2018 •

edited