feat: add endpoint to create cluster #41

xcoulon · 2018-12-04T15:45:03Z

endpoint access is limited to new toolchain-operator SA token
cluster service validates that the given cluster to create is valid
using cluster service mock to test the controller, to produce errors.

fixes #22

Signed-off-by: Xavier Coulon [email protected]

- endpoint access is limited to new `toolchain-operator` SA token - cluster service validates that the given cluster to create is valid - using cluster service mock to test the controller, to produce errors. fixes fabric8-services#22 Signed-off-by: Xavier Coulon <[email protected]>

xcoulon · 2018-12-04T15:47:01Z

@dipak-pawar @alexeykazakov the PR uses a mock of cluster service in the controller test, but that might be a bit of over-engineering, so feel free to provide me with feedback on that ;)
Also, the PR needs fabric8-services/fabric8-common#73 to be merged, first (I expect a few compilation errors on Jenkins for now)

alexeykazakov · 2018-12-04T18:29:57Z

@xcoulon we will also need to create credentials for the operator SA and add them to Auth configuration/secret. We will need an issue for that.

alexeykazakov

Yeah.. I think we don't really benefit from that service mock. We have to use mocks when services mare remote calls. But the cluster service talks to DB only. IMO regular integration tests is better here.

alexeykazakov · 2018-12-04T22:39:01Z

cluster/service/cluster_service.go

+// the `scheme` or `host` parts. Returns `true` otherwise.
+func validURL(urlStr string) (url.URL, bool) {
+	u, err := url.Parse(urlStr)
+	if err != nil || u.Scheme == "" || u.Host == "" {


Shouldn't we wrap the original error? Instead of returning bool I would return error and if it's not nil then use wrap it in errInvalidURLMsg. Otherwise we can lose important error details from url.Parse().

ok, let me see that

addressed in 59cdcb7

alexeykazakov · 2018-12-04T22:46:19Z

cluster/service/cluster_service.go

+	return *u, true
+}
+
+func forgeURL(baseURL url.URL, subdomain string) (string, error) {


You can use https://github.com/fabric8-services/fabric8-cluster/blob/master/cluster/cluster.go#L14 instead.
See an example of usage - https://github.com/fabric8-services/fabric8-cluster/blob/master/configuration/configuration.go#L191

ah right, I forgot about this function. Thanks for the pointer!

addressed in 59cdcb7

alexeykazakov · 2018-12-04T23:01:02Z

cluster/service/cluster_service.go

+		return errors.NewBadParameterErrorFromString(fmt.Sprintf(errEmptyFieldMsg, "service-account-username"))
+	}
+	if clustr.TokenProviderID == "" {
+		return errors.NewBadParameterErrorFromString(fmt.Sprintf(errEmptyFieldMsg, "token-provider-id"))


We actually could generate TokenProviderID if it's empty. TokenProviderID is used only by Auth service to associate user tokens obtained from the provider/cluster with that provider. We use provider ID because there are other token providers like GitHub.
So, TokenProviderID is basically some unique ID which will store in Auth external token DB when creating a token entry (so, we know that token came from that provider/cluster).
I would maybe set TokenProviderID (if empty) to the same value as ClusterID in our cluster repo, to minimize confusion we can have later on if we keep clusterID and TokenProviderID different.

ok, let me do that (in which case, the ID will be generated by the app, not the DB, but that's a minor side-effect)

addressed in 5467634

alexeykazakov · 2018-12-04T23:05:09Z

cluster/service/cluster_service_blackbox_test.go

+		// when
+		err := s.Application.ClusterService().CreateOrSaveCluster(context.Background(), &clustr)
+		// then
+		require.Error(t, err)


Check error message and type. You can use https://github.com/fabric8-services/fabric8-common/blob/b10e057d860d730661b2440dc1326c7d82606acc/test/error.go#L11 for that.

addressed in 545d475

alexeykazakov · 2018-12-04T23:05:52Z

cluster/service/cluster_service_blackbox_test.go

+		// then
+		require.Error(t, err)
+
+	})


Check all possible errors you can get (wrong URL, missing required params, etc)

this is going to be a duplicate of the validation tests that I did in https://github.com/fabric8-services/fabric8-cluster/pull/41/files#diff-e9581e303b0b0335c4bed88053faff86R20. Here, I just wanted to make sure that in case of a validation error, the service returns an error

You don't know if the validation is actually called. I would rather test the whole service which will test the internal validation function indirectly then relaying of testing implementation details of the service.

Your validate() function may work perfectly and all tests will pass but if this function is not used properly in the service or not used at all then the service won't work as expected but your tests won't catch it.
We have to make sure we test all exported functions very well with all possible outputs and usecases. Testing internal functions and implementation details is optional. Sometimes we have to test them because it's very tricky to test the exposed functions fully, so, it's better to test at least some the implementation parts (== not exposed functions) instead of the API (== exposed functions) than nothing. But it's not the case.

fair enough, I'll refactor the tests, because there's no point in having duplicate tests here, then

addressed in e54ba34

alexeykazakov · 2018-12-04T23:07:07Z

cluster/service/cluster_service_blackbox_test.go

+		err := s.Application.ClusterService().CreateOrSaveCluster(context.Background(), &clustr)
+		// then
+		require.NoError(t, err)
+		assert.NotNil(t, clustr.ClusterID)


Check the cluster was actually created. Check all the fields.
Also check if optional params are honored (consoleURL, etc are constructed as expected).

checking that optional fields are honored is already done in https://github.com/fabric8-services/fabric8-cluster/pull/41/files#diff-e9581e303b0b0335c4bed88053faff86R36, but ok, let me check that the data actually exists with a repository call

Same as #41 (comment)

addressed in e54ba34

all fields checked in 5467634

alexeykazakov · 2018-12-05T06:04:39Z

controller/clusters.go

+	clusterSvc := c.app.ClusterService()
+	err := clusterSvc.CreateOrSaveCluster(ctx, &clustr)
+	if err != nil {
+		return app.JSONErrorResponse(ctx, err)


Log the error.

yes, good point ;)

addressed in 0de0dbd

alexeykazakov · 2018-12-05T06:10:35Z

design/clusters.go

+		a.Description("Add a cluster configuration")
+		a.Response(d.Created)
+		a.Response(d.Unauthorized, JSONAPIErrors)
+		a.Response(d.InternalServerError, JSONAPIErrors)


Add 400 Bad Request. And also tests for that :)

good point, indeed!

done in 7ba3711

dipak-pawar · 2018-12-05T06:01:37Z

design/clusters.go

+		a.Description("Add a cluster configuration")
+		a.Response(d.Created)
+		a.Response(d.Unauthorized, JSONAPIErrors)
+		a.Response(d.InternalServerError, JSONAPIErrors)


missing a.Response(d.BadRequest, JSONAPIErrors)?

yes, as @alexeykazakov also pointed out. Let me fix that ;)

done in 7ba3711

dipak-pawar · 2018-12-05T06:11:45Z

design/clusters.go

@@ -13,6 +13,36 @@ var clusterList = JSONList(
 	nil,
 	nil)

+// singleCluster represents a single cluster object


probably...

addressed in 59369a0

dipak-pawar · 2018-12-05T06:11:53Z

design/clusters.go

+// singleCluster represents a single cluster object
+var createCluster = JSONSingle(
+	"Cluster",
+	"Holds the response to a single cluster request",


it's not response, holds the data attributes to create cluster

ok, let me fix that

addressed in 59369a0

dipak-pawar · 2018-12-05T06:14:59Z

design/clusters.go

+		"app-dns", "type",
+		"service-account-token", "service-account-username", "token-provider-id", "auth-client-id", "auth-client-secret",
+		"auth-client-default-scope")
+})


IMPO, we should user - -> _ e.g. auth-client-secret -> auth_client_secret, as we are already at initial stage and let's have uniform way, so everywhere we'll use _. If you agree on it I will update existing code.

cc @alexeykazakov

I'm not sure what is better to use _ or - but I fully agree that we should be consistent.

I thought we agreed on - last week ? See the whole content of https://github.com/fabric8-services/fabric8-cluster/blob/79d13b3f4773af3e71bbbeb8eebf5c6b631a044d/design/clusters.go

yes, we agreed on -. I know I have preference _ over -. Just wanted to bring alexey's attention

@dipak-pawar yes, consistency across the API design is what matters the most to me too (I have a personal preference for - over _ but it's a personal taste, of course).
Now, if we decide to change the other endpoints' attributes pattern from - to _, we'll also need to redeploy all the other services with an updated client lib at the same time, otherwise all requests will fail. So, for the sake of simplicity, I suggest we carry on with - to avoid troubles with the exists deployments. Ok for you?

Guys, just checked and we are very bad in consistency :) We use _, -, camelCase all over the place. I mean in all our services. Not just here.
I do not have any preferences but let's pick something and try to stick with that. BTW is there any guidelines, good practices, etc, or it's just pure taste thing?

dipak-pawar · 2018-12-05T06:31:57Z

cluster/service/cluster_service.go

+
+// validate checks if all data in the given cluster is valid, and fills the missing/optional URLs using the `APIURL`
+func validate(clustr *repository.Cluster) error {
+	if clustr.Name == "" {


There is no need to validate empty params if you are mentioning it as required in design, because goa will handle it e.g.
{"errors":[{"code":"bad_request","detail":"[zzwRTz/z] 400 invalid_request: attribute \"name\" of request is missing and required; parent: request","id":"CwNtrPHC","status":"400","title":"Bad Request"}]}

It's same for all below empty validation.

are you sure about that ? you're showing a JSON-API response, but I'm not sure Goa returns such a thing by default. Anyways, I'll double-check that

yes, I am sure about it. Maybe we can check by trimming fields, to catch by some weired request with non-empty space, like " "

good point for trimming spaces from the fields, indeed. But what about the response format?

addressed in 133b68b

xcoulon · 2018-12-05T07:48:54Z

@alexeykazakov ok, so I will remove the minimock parts and just rely on the actual service when testing the endpoint.

xcoulon · 2018-12-05T08:51:40Z

@xcoulon we will also need to create credentials for the operator SA and add them to Auth configuration/secret. We will need an issue for that.

@alexeykazakov ok, I just opened fabric8-services/fabric8-auth#723 for that matter. We'll need to fill an HK for SD teams, too.

Signed-off-by: Xavier Coulon <[email protected]>

… the new 'auth.ToolChainOperator' const Signed-off-by: Xavier Coulon <[email protected]>

codecov · 2018-12-05T10:07:21Z

Codecov Report

Merging #41 into master will increase coverage by 1.76%.
The diff coverage is 78.89%.

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage    73.5%   75.27%   +1.76%     
==========================================
  Files          15       15              
  Lines         872      991     +119     
==========================================
+ Hits          641      746     +105     
- Misses        192      197       +5     
- Partials       39       48       +9

Impacted Files	Coverage Δ
application/service/factory/service_factory.go	`60% <ø> (+1.53%)`	⬆️
cluster/cluster.go	`60% <100%> (ø)`	⬆️
configuration/configuration.go	`79.09% <100%> (+0.52%)`	⬆️
cluster/repository/cluster_repository.go	`69.81% <20%> (ø)`
cluster/service/cluster_service.go	`79.14% <79.14%> (ø)`
controller/clusters.go	`90.12% <80.95%> (-9.88%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 687476d...c5319b2. Read the comment docs.

Signed-off-by: Xavier Coulon <[email protected]>

dipak-pawar · 2018-12-05T11:11:10Z

.gitignore

@@ -78,4 +78,4 @@ tool/
 migration/sqlbindata.go
 configuration/confbindata.go
 wit/witservice
-account/tenant
+account/tenant


Add test/generated if we are using mini mock, if not then let's forget

are you asking me to restore the entry in .gitignore ?

no, forget it as we are not generating any mock for tests.

Signed-off-by: Xavier Coulon <[email protected]>

alexeykazakov · 2018-12-05T17:10:30Z

cluster/service/cluster_service.go

@@ -147,21 +161,15 @@ func validate(clustr *repository.Cluster) error {

 // validateURL validates the URL: return `false` if the given url could not be parsed or if it is missing


Need to fix the comment. It returns error now.

fixed in 790c3b0

alexeykazakov · 2018-12-05T17:21:50Z

cluster/service/cluster_service_whitebox_test.go

@@ -145,7 +144,7 @@ func TestValidation(t *testing.T) {
 				// then
 				require.Error(t, err)
 				require.IsType(t, errors.BadParameterError{}, err)
-				assert.Equal(t, fmt.Sprintf(errInvalidURLMsg, "API", c.URL), err.(errors.BadParameterError).Error())
+				assert.Equal(t, "'API' URL '' is invalid: missing scheme or host", err.(errors.BadParameterError).Error())


Why still not using https://github.com/fabric8-services/fabric8-common/blob/b10e057d860d730661b2440dc1326c7d82606acc/test/error.go#L11 here and in other places? You could use just one statement instead of three asserts/requires (error not nil, type, mesage). See an example of usage here - https://github.com/fabric8-services/fabric8-common/blob/b10e057d860d730661b2440dc1326c7d82606acc/auth/service_blackbox_test.go#L70

ok, let me do that

addressed in af4ee21

alexeykazakov · 2018-12-05T17:28:15Z

@xcoulon another important thing.. We should make sure we are consistent in storing URLs with trailing slashes or without them in our DB to avoid situations when we have "https://api.somecluster.com/" for some clusters and "https://api.anothercluster.com" for others. It will complicate our select queries. Let's make sure we add or remove all trailing slashes in all URLs in the cluster when storing them in DB. I guess it's better to do in cluster repo to make sure we don't miss a thing.
WDYT?
cc: @dipak-pawar

…ovided by request also, check all fields in cluster upon creation by service Signed-off-by: Xavier Coulon <[email protected]>

Signed-off-by: Xavier Coulon <[email protected]>

xcoulon · 2018-12-05T17:44:41Z

@xcoulon another important thing.. We should make sure we are consistent in storing URLs with trailing slashes or without them in our DB to avoid situations when we have "https://api.somecluster.com/" for some clusters and "https://api.anothercluster.com" for others. It will complicate our select queries. Let's make sure we add or remove all trailing slashes in all URLs in the cluster when storing them in DB. I guess it's better to do in cluster repo to make sure we don't miss a thing.
WDYT?
cc: @dipak-pawar

what about URLs such as https://console.cluster-foo.com/console/ ? I believe that in some cases, we should keep the trailing /, shouldn't we ?

alexeykazakov · 2018-12-05T17:49:41Z

cluster/service/cluster_service.go

+	}
+	if strings.TrimSpace(clustr.TokenProviderID) == "" {
+		// generated a value based on the ID of this cluster, so it's easier to track
+		clustr.ClusterID = uuid.NewV4()


I don't think we can set ClusterID here. CreateOrSave() will try to update the cluster if it exists. And we can't change its ID. It will fail to update then. I guess we have to load the cluster by API URL here and if it exists then use this ID?

good point. Let me run a test to verify what happens when an existing cluster is updated, but no TokenProviderID value is given

addressed in 638ecb7

alexeykazakov · 2018-12-05T17:57:33Z

@xcoulon there are two things. What we store in our DB and what we accept and return in our REST API.
Our REST API always returns all URLs with trailing slashes - https://github.com/fabric8-services/fabric8-cluster/blob/master/controller/clusters.go#L42-L45
We also should accept requests with both trailing slashes or without them.
The other thing is how we actually store them in our DB. We should always add or remove the trailing slashes so in DB it's consistent. Then while doing selects we can normal the query value to the URL format stored in the DB.
Does it make sense?

Let's say we always add trailing slashes when storing in DB. Then we know if some client wants to get a cluster for some URL we just have to add the trailing slash to that URL and execute a select.
Or wise verse. If we always store URLs without slashes we just remove the trialing slash from the request URL and do a select.

xcoulon · 2018-12-06T08:04:57Z

@xcoulon there are two things. What we store in our DB and what we accept and return in our REST API.
Our REST API always returns all URLs with trailing slashes - https://github.com/fabric8-services/fabric8-cluster/blob/master/controller/clusters.go#L42-L45
We also should accept requests with both trailing slashes or without them.
The other thing is how we actually store them in our DB. We should always add or remove the trailing slashes so in DB it's consistent. Then while doing selects we can normal the query value to the URL format stored in the DB.
Does it make sense?

Let's say we always add trailing slashes when storing in DB. Then we know if some client wants to get a cluster for some URL we just have to add the trailing slash to that URL and execute a select.
Or wise verse. If we always store URLs without slashes we just remove the trialing slash from the request URL and do a select.

sure, that makes sense! Let me address this with a few tests to cover all case :)

Signed-off-by: Xavier Coulon <[email protected]>

dipak-pawar · 2018-12-06T11:52:32Z

cluster/repository/cluster_repository.go

@@ -34,12 +34,12 @@ type Cluster struct {
 	LoggingURL string
 	// Application host name used by the cluster
 	AppDNS string
-	// Service Account token
-	SaToken string
+	// Encrypted Service Account token


We don't know, it's encrypted or not by looking at this field. we need to look at SATokenEncrypted

addressed in eb0aa0a

Signed-off-by: Xavier Coulon <[email protected]>

xcoulon · 2018-12-06T12:01:58Z

@xcoulon there are two things. What we store in our DB and what we accept and return in our REST API.
Our REST API always returns all URLs with trailing slashes - https://github.com/fabric8-services/fabric8-cluster/blob/master/controller/clusters.go#L42-L45
We also should accept requests with both trailing slashes or without them.
The other thing is how we actually store them in our DB. We should always add or remove the trailing slashes so in DB it's consistent. Then while doing selects we can normal the query value to the URL format stored in the DB.
Does it make sense?
Let's say we always add trailing slashes when storing in DB. Then we know if some client wants to get a cluster for some URL we just have to add the trailing slash to that URL and execute a select.
Or wise verse. If we always store URLs without slashes we just remove the trialing slash from the request URL and do a select.

sure, that makes sense! Let me address this with a few tests to cover all case :)

@alexeykazakov addressed in fbe2a7f

Signed-off-by: Xavier Coulon <[email protected]>

alexeykazakov

Great work @xcoulon!
Just a few minor comments.

alexeykazakov · 2018-12-06T17:46:50Z

cluster/repository/cluster_repository.go

@@ -170,7 +170,12 @@ func (m *GormClusterRepository) CreateOrSave(ctx context.Context, c *Cluster) er
 	obj, err := m.LoadClusterByURL(ctx, c.URL)
 	if err != nil {
 		if ok, _ := errors.IsNotFoundError(err); ok {
-			return m.Create(ctx, c)
+			err = m.Create(ctx, c)
+			log.Info(ctx, map[string]interface{}{


Well, if err!=nil then the cluster has not been created. If you want to log successful creation then use logging in Create() instead?

good point!
and since the Create method already logs a message upon successful creation or error, I'm going to remove it here ;)

alexeykazakov · 2018-12-06T18:30:43Z

cluster/service/cluster_service.go

+		return fmt.Errorf("missing scheme or host")
+	}
+	// make sure that the URL ends with a slash in all cases
+	*urlStr = httpsupport.AddTrailingSlashToURL(*urlStr)


validate() is a bit confusing name for this function now. It's not just validation anymore. Shouldn't we call it validateAndNormalize() or something?

sound good. let me rename this func to validateAndNormalize

alexeykazakov · 2018-12-06T18:43:51Z

controller/clusters_blackbox_test.go

+}
+
+func newCreateClusterPayload() app.CreateClustersPayload {
+	random := uuid.NewV4().String()


Why to use the same value for all fields? Isn't it safer to use random and different field values in the test? Then we can differ them in the tests.

ok, let me (partially) revert that one

Signed-off-by: Xavier Coulon <[email protected]>

xcoulon requested review from alexeykazakov and dipak-pawar December 4, 2018 15:45

alexeykazakov suggested changes Dec 5, 2018

View reviewed changes

dipak-pawar reviewed Dec 5, 2018

View reviewed changes

xcoulon added 4 commits December 5, 2018 10:38

apply review comments

59cdcb7

Signed-off-by: Xavier Coulon <[email protected]>

apply review comments

545d475

Signed-off-by: Xavier Coulon <[email protected]>

apply review comments

0de0dbd

Signed-off-by: Xavier Coulon <[email protected]>

upgrade common to 2efd83d29b1e5c657728e83fe03878569c0f122f to resolve…

0ca6cb0

… the new 'auth.ToolChainOperator' const Signed-off-by: Xavier Coulon <[email protected]>

xcoulon added 3 commits December 5, 2018 11:42

remove usage of mock to test cluster endpoint

7ba3711

Signed-off-by: Xavier Coulon <[email protected]>

apply review comments

59369a0

Signed-off-by: Xavier Coulon <[email protected]>

trim spaces on fields during validation

133b68b

Signed-off-by: Xavier Coulon <[email protected]>

dipak-pawar reviewed Dec 5, 2018

View reviewed changes

xcoulon added 2 commits December 5, 2018 12:11

Merge branch 'master' into Issue22_endpoint_add_cluster

95fe200

test cluster data validation from blackbox test

e54ba34

Signed-off-by: Xavier Coulon <[email protected]>

alexeykazakov reviewed Dec 5, 2018

View reviewed changes

xcoulon added 4 commits December 5, 2018 18:31

make 'TokenProviderID' field optional, fill with cluster ID if not pr…

5467634

…ovided by request also, check all fields in cluster upon creation by service Signed-off-by: Xavier Coulon <[email protected]>

use AssertError(..) to avoid duplicate statements

af4ee21

Signed-off-by: Xavier Coulon <[email protected]>

fix method comment

790c3b0

Signed-off-by: Xavier Coulon <[email protected]>

Merge branch 'master' into Issue22_endpoint_add_cluster

c5e5989

alexeykazakov reviewed Dec 5, 2018

View reviewed changes

default value for TokenProviderID at creation

638ecb7

Signed-off-by: Xavier Coulon <[email protected]>

dipak-pawar reviewed Dec 6, 2018

View reviewed changes

add trailing slashes when creating/saving clusters

fbe2a7f

Signed-off-by: Xavier Coulon <[email protected]>

xcoulon added 3 commits December 6, 2018 15:05

test fixes

e34636b

Signed-off-by: Xavier Coulon <[email protected]>

go fmt...

676554c

Signed-off-by: Xavier Coulon <[email protected]>

apply review comments

eb0aa0a

Signed-off-by: Xavier Coulon <[email protected]>

alexeykazakov approved these changes Dec 6, 2018

View reviewed changes

apply review comments

c5319b2

Signed-off-by: Xavier Coulon <[email protected]>

xcoulon merged commit 80c7fb3 into fabric8-services:master Dec 7, 2018

xcoulon deleted the Issue22_endpoint_add_cluster branch December 7, 2018 08:01

		@@ -147,21 +161,15 @@ func validate(clustr *repository.Cluster) error {

		// validateURL validates the URL: return `false` if the given url could not be parsed or if it is missing

feat: add endpoint to create cluster #41

feat: add endpoint to create cluster #41

Conversation

xcoulon commented Dec 4, 2018

xcoulon commented Dec 4, 2018

alexeykazakov commented Dec 4, 2018

alexeykazakov 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

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

dipak-pawar Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xcoulon Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xcoulon commented Dec 5, 2018

xcoulon commented Dec 5, 2018

codecov bot commented Dec 5, 2018 • edited Loading

Codecov Report

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

alexeykazakov commented Dec 5, 2018

xcoulon commented Dec 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeykazakov commented Dec 5, 2018

xcoulon commented Dec 6, 2018

Choose a reason for hiding this comment

dipak-pawar Dec 5, 2018 •

edited

Loading

xcoulon Dec 5, 2018 •

edited

Loading

codecov bot commented Dec 5, 2018 •

edited

Loading