-
Notifications
You must be signed in to change notification settings - Fork 5
feat: read clusters from database #56
feat: read clusters from database #56
Conversation
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 Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. But please take a look at the comments below.
.make/test.mk
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in dc31c0f
@@ -236,3 +238,9 @@ func (m *GormClusterRepository) Query(funcs ...func(*gorm.DB) *gorm.DB) ([]Clust | |||
|
|||
return objs, nil | |||
} | |||
|
|||
// List list ALL clusters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// List lists ALL clusters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in dc31c0f
|
||
func (s *clusterTestSuite) TestList() { | ||
// given | ||
cluster1 := &repository.Cluster{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just use https://github.com/fabric8-services/fabric8-cluster/blob/master/test/cluster.go#L18 to create a random cluster in DB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in dc31c0f
// then | ||
require.NoError(s.T(), err) | ||
require.Len(s.T(), clusters, 2) | ||
test.AssertEqualClusters(s.T(), cluster1, &clusters[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, let me fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in dc31c0f
cluster/service/cluster_service.go
Outdated
} | ||
|
||
go func() { | ||
fmt.Println("config watcher started") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove Println
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in d78a591
cluster/service/cluster_service.go
Outdated
} | ||
|
||
go func() { | ||
fmt.Println("config watcher started") | ||
defer func() { | ||
fmt.Println("config watcher stopped") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove println
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in d78a591
cluster/service/cluster_service.go
Outdated
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really still need that done channel in the tests after fixing the parallel test execution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, it's not needed anymore, so I reverted that part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in d78a591
controller/clusters_blackbox_test.go
Outdated
require.NotNil(t, clusters) | ||
require.NotNil(t, clusters.Data) | ||
require.Len(t, clusters.Data, len(s.Configuration.GetClusters())) | ||
for _, cluster := range clusters.Data { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let me add that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in dc31c0f
controller/clusters_blackbox_test.go
Outdated
require.NotNil(t, clusters) | ||
require.NotNil(t, clusters.Data) | ||
require.Equal(t, len(s.Configuration.GetClusters()), len(clusters.Data)) | ||
for _, cluster := range clusters.Data { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in dc31c0f
test/cluster.go
Outdated
Name: uuid.NewV4().String(), | ||
SAToken: uuid.NewV4().String(), | ||
SAUsername: uuid.NewV4().String(), | ||
ClusterID: random, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in ae521c6
thanks for the review @alexeykazakov and let me address all your comments |
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
@alexeykazakov I addressed all your comments. Please let me know if you have further concerns ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great Job @xcoulon. LGTM
cluster/service/cluster_service.go
Outdated
if err != nil { | ||
return err | ||
} | ||
} | ||
log.Warn(ctx, map[string]interface{}{}, "done creating/updating clusters from config file") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about creating/updating clusters from config file has been completed/done
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change the message if you prefer that wording
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in e99d1ef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just the last comment regarding the trailing slashes.
// 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
@dipak-pawar @alexeykazakov please let me know if you have further concerns about this PR ;) |
test/cluster.go
Outdated
type NormalizeCluster func(repository.Cluster) repository.Cluster | ||
|
||
// AddTrailingSlashes a normalize function which converts all URL by adding a trailing | ||
// slash if neede |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in fa40eab
} | ||
|
||
// AssertEqualClusterDetails verifies that the `actual` and `expected` clusters are have the same values | ||
// including sensitive details if `expectSensitiveInfo` is `true` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mentioned expectSensitiveInfo
param in a few places but it looks like it's not defined/actually used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in fa40eab
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
also:
fabric8-common
to remove an annoying errormessage after each test
writes in the database, which result in assertion errors
Fixes #54
Signed-off-by: Xavier Coulon xcoulon@redhat.com