KSPACE-43: Adding common GetClusters func #372
Conversation
Signed-off-by: Feny Mehta <[email protected]>
fbm3307
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19,
sbryzak and
mfrancisc
as code owners
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #372 +/- ##
==========================================
- Coverage 75.99% 75.46% -0.54%
==========================================
Files 44 44
Lines 1683 1683
==========================================
- Hits 1279 1270 -9
- Misses 353 362 +9
Partials 51 51
|
| // GetHostClusterFunc a func that returns the Host cluster from the cache, | ||
| // along with a bool to indicate if there was a match or not | ||
| type GetHostClusterFunc func() (*CachedToolchainCluster, bool) |
There was a problem hiding this comment.
don't forget to remove the GetHostCluster, GetMemberClusters, and the related functions and types - they shouldn't be used anymore.
There was a problem hiding this comment.
ok, I see that you wrote
Keeping the GetHostCluster() and GetMemberClusters() for now and delete them once their dependencies are removed in host, member, registration-service .
but let's delete it right away in this PR, so we are sure that we didn't miss any usage anywhere
pkg/status/toolchaincluster.go
Outdated
| if !ok { | ||
| toolchainClusters := attrs.GetClustersFunc() | ||
|
|
||
| if len(toolchainClusters) != 1 { |
There was a problem hiding this comment.
originally, there was:
if !ok {which is not then same as
if len(toolchainClusters) != 1 {when there can be 0...n results 🙃
pkg/status/toolchaincluster.go
Outdated
| GetClusterFunc func() (*cluster.CachedToolchainCluster, bool) | ||
| Period time.Duration | ||
| Timeout time.Duration | ||
| GetClustersFunc cluster.GetClustersFunc |
There was a problem hiding this comment.
the purpose and the usage of the ToolchainClusterAttributes type and GetToolchainClusterConditions function doesn't match with the cluster.GetClustersFunc - this is intended to be used for one ToolchainCluster, but for many. I saw that you used
toolchainCluster := toolchainClusters[0]to get the first one, but you never know if you are supposed to use the first one, second, or third.
I would rather either keep the
GetClusterFunc func() (*cluster.CachedToolchainCluster, bool)in the same format is it was before and let the caller to pass the ToolchainCluster - something like this:
attributes := status.ToolchainClusterAttributes{
GetClusterFunc: func() (*cluster.CachedToolchainCluster, bool) {
clusters := cluster.GetClustersFunc()
if len(clusters) != 0 {
return clusters[0], true
}
return nil, false
},or just change the field so it doesn't take a func but rather a *cluster.CachedToolchainCluster directly
pkg/cluster/cache.go
Outdated
| var Clusters GetClustersFunc = GetClusters | ||
|
|
||
| // GetClusters returns the kube clients for all the clusters from the cache of the clusters | ||
| func GetClusters(conditions ...Condition) []*CachedToolchainCluster { |
There was a problem hiding this comment.
one more thing, GetClusters is pretty generic and can be confusing. How about:
- GetToolchainClusters
- GetCachedToolchainClusters
so it's clear which clusters we are talking about
There was a problem hiding this comment.
There is already a function named as GetCachedToolchainClusters , but to avoid confusion and for better understanding updating it to GetToolchainClustersCachedFunc
Signed-off-by: Feny Mehta <[email protected]>
|
|
Closing this PR as Adding the Common Getcluster Func is not required now |
This is a Follow-up PR for #359,
GetClusters()to fetch the clusters from cache as we have dropped the cluster typeGetHostCluster()andGetMemberClusters()GetHostCluster()andGetMemberClusters()for now and delete them once their dependencies are removed in host, member, registration-service .