feat: read clusters from database #56

xcoulon · 2019-01-04T10:11:46Z

also:

update to latest fabric8-common to remove an annoying error
message after each test
run integration tests on a single processor to avoid concurrent
writes in the database, which result in assertion errors

Fixes #54

Signed-off-by: Xavier Coulon xcoulon@redhat.com

also: - update to latest `fabric8-common` to remove an annoying error message after each test - run integration tests on a single processor to avoid concurrent writes in the database, which result in assertion errors Fixes fabric8-services#54 Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

codecov · 2019-01-04T10:40:40Z

Codecov Report

Merging #56 into master will decrease coverage by 0.03%.
The diff coverage is 92.72%.

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
- Coverage   78.57%   78.53%   -0.04%     
==========================================
  Files          15       15              
  Lines        1111     1123      +12     
==========================================
+ Hits          873      882       +9     
- Misses        190      192       +2     
- Partials       48       49       +1

Impacted Files	Coverage Δ
cluster/repository/identity_cluster.go	`83.92% <ø> (ø)`	⬆️
controller/user.go	`76.92% <100%> (ø)`	⬆️
cluster/repository/cluster_repository.go	`70.09% <100%> (+0.28%)`	⬆️
cluster/service/cluster_service.go	`80.58% <100%> (+0.77%)`	⬆️
controller/clusters.go	`92.89% <90%> (-2.14%)`	⬇️

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 6a19264...fa40eab. Read the comment docs.

alexeykazakov

Overall looks good. But please take a look at the comments below.

alexeykazakov · 2019-01-05T00:03:00Z

.make/test.mk

@@ -147,6 +147,7 @@ test-unit-with-coverage: prebuild-check clean-coverage-unit $(COV_PATH_UNIT)
 test-unit: prebuild-check $(SOURCES)
 	$(call log-info,"Running test: $@")
 	$(eval TEST_PACKAGES:=$(shell go list ./... | grep -v $(ALL_PKGS_EXCLUDE_PATTERN)))
+	go clean -cache


Why do you want to clean cache? Adding -p 1 flag slows down tests. So, cache is actually quite useful when running tests locally. You can always clean cache manually between test runs if you want.

right, let me remove that. I just wanted to make sure that there was no cache when running the integration tests to reproduce the "parallel run" failures, but I can surely remove that one now.

done in dc31c0f

alexeykazakov · 2019-01-05T00:05:34Z

cluster/repository/cluster_repository.go

@@ -236,3 +238,9 @@ func (m *GormClusterRepository) Query(funcs ...func(*gorm.DB) *gorm.DB) ([]Clust

 	return objs, nil
 }
+
+// List list ALL clusters


// List lists ALL clusters

done in dc31c0f

alexeykazakov · 2019-01-05T00:11:27Z

cluster/repository/cluster_repository_blackbox_test.go

+
+func (s *clusterTestSuite) TestList() {
+	// given
+	cluster1 := &repository.Cluster{


You can just use https://github.com/fabric8-services/fabric8-cluster/blob/master/test/cluster.go#L18 to create a random cluster in DB.

cool, thanks!

done in dc31c0f

alexeykazakov · 2019-01-05T00:13:59Z

cluster/repository/cluster_repository_blackbox_test.go

+	// then
+	require.NoError(s.T(), err)
+	require.Len(s.T(), clusters, 2)
+	test.AssertEqualClusters(s.T(), cluster1, &clusters[0])


You do not order clusters when query them in DB, so, theoretically they can be in a different order. Worth adding test.AssertClusters(t, expected, actual []Cluster) or test.AssertClusters(t, actual []Cluster, expected ...Cluster) since it can be useful in other tests too.

good point, let me fix that

done in dc31c0f

alexeykazakov · 2019-01-05T00:21:48Z

cluster/service/cluster_service.go

 	}

 	go func() {
+		fmt.Println("config watcher started")


remove Println

done in d78a591

alexeykazakov · 2019-01-05T00:22:51Z

cluster/service/cluster_service.go

 	}

 	go func() {
+		fmt.Println("config watcher started")
+		defer func() {
+			fmt.Println("config watcher stopped")


remove println

done in d78a591

alexeykazakov · 2019-01-05T00:24:13Z

cluster/service/cluster_service.go

@@ -217,13 +217,20 @@ func ValidateURL(urlStr *string) error {

 // InitializeClusterWatcher initializes a file watcher for the cluster config file
 // When the file is updated the configuration synchronously reload the cluster configuration
-func (c clusterService) InitializeClusterWatcher() (func() error, error) {
+func (c clusterService) InitializeClusterWatcher() (func() error, chan bool, error) {


Do you really still need that done channel in the tests after fixing the parallel test execution?

right, it's not needed anymore, so I reverted that part.

done in d78a591

alexeykazakov · 2019-01-05T00:36:30Z

controller/clusters_blackbox_test.go

+				require.NotNil(t, clusters)
+				require.NotNil(t, clusters.Data)
+				require.Len(t, clusters.Data, len(s.Configuration.GetClusters()))
+				for _, cluster := range clusters.Data {


Also add at least on cluster to the DB manually and make sure it's among the result. Not only the clusters loaded from Configuration.

ok, let me add that

done in dc31c0f

alexeykazakov · 2019-01-05T00:37:00Z

controller/clusters_blackbox_test.go

+			require.NotNil(t, clusters)
+			require.NotNil(t, clusters.Data)
+			require.Equal(t, len(s.Configuration.GetClusters()), len(clusters.Data))
+			for _, cluster := range clusters.Data {


Same as above.

done in dc31c0f

alexeykazakov · 2019-01-05T00:46:25Z

test/cluster.go

-		Name:              uuid.NewV4().String(),
-		SAToken:           uuid.NewV4().String(),
-		SAUsername:        uuid.NewV4().String(),
+		ClusterID:         random,


Guys, you keep "optimizing" this code by using the same string for fields which are supposed to be unique :) The whole point here that these fields have different value. Not the same.

I get your point, I just wanted to have a same UUID for all fields so I could much easily locate the cluster ID in the logs. Let me revert that now that I don't need it anymore

done in ae521c6

xcoulon · 2019-01-07T07:38:49Z

thanks for the review @alexeykazakov and let me address all your comments

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

xcoulon · 2019-01-07T09:36:16Z

@alexeykazakov I addressed all your comments. Please let me know if you have further concerns ;)

dipak-pawar

Great Job @xcoulon. LGTM

dipak-pawar · 2019-01-07T15:41:12Z

cluster/service/cluster_service.go

 		if err != nil {
 			return err
 		}
 	}
+	log.Warn(ctx, map[string]interface{}{}, "done creating/updating clusters from config file")


How about creating/updating clusters from config file has been completed/done?

I can change the message if you prefer that wording

done in e99d1ef

alexeykazakov

Looks good! Just the last comment regarding the trailing slashes.

alexeykazakov · 2019-01-07T18:15:50Z

test/cluster.go

+// AssertEqualClusterData verifies that data for actual cluster match the expected one
+func AssertEqualClusterData(t *testing.T, expected *repository.Cluster, actual *app.ClusterData) {
+	assert.Equal(t, expected.Name, actual.Name)
+	assert.Equal(t, httpsupport.AddTrailingSlashToURL(expected.URL), actual.APIURL)


Wouldn't it be safer to check URLs as is instead of adding trailing slashes to expected URLs?
We should be really careful with these trailing slashes. It's easy to miss it.

done in 238c902 with a Normalize function outside of the AssertEqualCluster funcs

also, include `SaTokenEncrypted` in response also, refactor custom assertions to simplify code in tests Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

xcoulon · 2019-01-08T10:22:37Z

@dipak-pawar @alexeykazakov please let me know if you have further concerns about this PR ;)

alexeykazakov · 2019-01-08T16:58:37Z

test/cluster.go

+type NormalizeCluster func(repository.Cluster) repository.Cluster
+
+// AddTrailingSlashes a normalize function which converts all URL by adding a trailing
+// slash if neede


fixed in fa40eab

alexeykazakov · 2019-01-08T17:02:00Z

test/cluster.go

 }

+// AssertEqualClusterDetails verifies that the `actual` and `expected` clusters are have the same values
+// including sensitive details if `expectSensitiveInfo` is `true`


You mentioned expectSensitiveInfo param in a few places but it looks like it's not defined/actually used anywhere?

well, that's the result of a copy/paste from an upcoming PR that actually introduces this arg, but it's not needed here, so I'll remove it from the comment

fixed in fa40eab

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

xcoulon requested review from dipak-pawar and alexeykazakov January 4, 2019 10:12

alexeykazakov suggested changes Jan 5, 2019

View reviewed changes

dipak-pawar mentioned this pull request Jan 7, 2019

Update Show and ShowClient to use clusters from DB instead of config #46

Closed

xcoulon added 4 commits January 7, 2019 08:39

Merge branch 'master' into Issue54_list_cluster_from_repository

31e4836

Apply review comments

dc31c0f

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

Apply review comments

ae521c6

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

Apply review comments

d78a591

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

dipak-pawar approved these changes Jan 7, 2019

View reviewed changes

alexeykazakov reviewed Jan 7, 2019

View reviewed changes

xcoulon added 2 commits January 8, 2019 11:18

normalize URLs

238c902

also, include `SaTokenEncrypted` in response also, refactor custom assertions to simplify code in tests Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

apply review comments

e99d1ef

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

alexeykazakov approved these changes Jan 8, 2019

View reviewed changes

fix function comments

fa40eab

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

xcoulon merged commit 7b582d0 into fabric8-services:master Jan 9, 2019

xcoulon deleted the Issue54_list_cluster_from_repository branch January 9, 2019 08:40

feat: read clusters from database #56

feat: read clusters from database #56

Conversation

xcoulon commented Jan 4, 2019 • edited Loading

codecov bot commented Jan 4, 2019 • edited Loading

Codecov Report

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

xcoulon commented Jan 7, 2019

xcoulon commented Jan 7, 2019

dipak-pawar 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

alexeykazakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xcoulon Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

xcoulon commented Jan 8, 2019

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

xcoulon commented Jan 4, 2019 •

edited

Loading

codecov bot commented Jan 4, 2019 •

edited

Loading

xcoulon Jan 8, 2019 •

edited

Loading