Add long-running test cases & implement slice merge for ReplicationSpecs #138
Conversation
vasilevp
changed the title
|
Maybe this PR fixed error described here: #126 I think the problem it is, more attributes are being sent to the API during the cluster update and it is not necessary. |
antonlisovenko
left a comment
antonlisovenko
left a comment
There was a problem hiding this comment.
This looks great and the generic approach to slices is a good one (strong reflection 👍 )
I have some suggestions for unit and int tests - tell me what you think?
| clusterMerged := mongodbatlas.Cluster{} | ||
| if err := compat.JSONCopy(&clusterMerged, cluster); err != nil { | ||
| return false, err | ||
| func cleanupCluster(cluster mongodbatlas.Cluster) mongodbatlas.Cluster { |
There was a problem hiding this comment.
[nit] can we add some comment about why we need to clean these fields?
|
|
||
| if err := compat.JSONCopy(&clusterMerged, spec); err != nil { | ||
| return false, err | ||
| // TODO: might need to do this with other slices |
There was a problem hiding this comment.
can we file a ticket for this? Seems like we need to cover all slices with tests...
|
|
||
| // merge common elements | ||
| for i := 0; i < minLen; i++ { | ||
| dstX := dstVal.Index(i).Addr().Interface() |
There was a problem hiding this comment.
[q] am I right that this will work only if the items of the slice are pointers as well? (looking at the tests below seems only the destination slice must have pointers?)
If so - may need to add to the method documentation
There was a problem hiding this comment.
They don't have to be pointers - I used pointers in the test case just to check that pointers work too
| . "github.com/mongodb/mongodb-atlas-kubernetes/pkg/util/compat" | ||
| ) | ||
|
|
||
| func TestJSONSliceMerge(t *testing.T) { |
There was a problem hiding this comment.
Great to have the unit test!
Do you think we can add the use cases:
- when the src has smaller size than the dst?
- the src is empty/nil
- the dst is empty/nil
test/int/cluster_test.go
Outdated
| checkAtlasState() | ||
| }) | ||
|
|
||
| By("Increasing DiskSizeGB", func() { |
There was a problem hiding this comment.
[q] do you think we need the separate test for increasing DiscSize if we have the one decreasing it? https://github.com/mongodb/mongodb-atlas-kubernetes/blob/2ba4112c78a3257973e5c01dcaeb95ad8569fcf3/test/int/cluster_test.go#L189-L188
There was a problem hiding this comment.
This one takes a lot of time due to disk provisioning, so I decided against including it in the "main" test suite
There was a problem hiding this comment.
as discussed p2p it may make more sense to cover something different (like providerSettings.instanceSizeName or whatever) as DiscSize is already covered in the other testcase)
| lastGeneration++ | ||
| } | ||
|
|
||
| Describe("Create cluster & change ReplicationSpecs", func() { |
There was a problem hiding this comment.
What do you think about adding another a bit more complicated use case and specifying two different regions for one replicationSpec (and some configuration for analytics/read-only nodes)?
The current use-case is nice as it covers our merging logic (take the things from Atlas and merge ours on top of the default ones) though we don't have the coverage for more complicated scenario when the user spreads their cluster over regions)
I doubt this PR will fix the issue right away - but it's the necessary first step to support this specific use case, among many others. |
antonlisovenko
left a comment
antonlisovenko
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments and happy to have good coverage of replicationSpecs!
There's another ticket for global clusters - I think we can test them later.
Well done 👍
| By("Change cluster to GEOSHARDED", func() { | ||
| createdCluster.Spec.ClusterType = "GEOSHARDED" | ||
| createdCluster.Spec.ProviderSettings.RegionName = "" | ||
| createdCluster.Spec.ReplicationSpecs = []mdbv1.ReplicationSpec{ |
There was a problem hiding this comment.
[q] does this deploy 3 nodes in east_us and 0 - in west_us?
There was a problem hiding this comment.
You're right - should I change it to 2/1 instead of 3/0?
There was a problem hiding this comment.
yes, 2/1 looks good to me
What do you think if we add 1 readonly/analytics node to some region?
No description provided.