Refactor/cleanup #962
Refactor/cleanup #962
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
Codecov Report
@@ Coverage Diff @@
## main #962 +/- ##
==========================================
+ Coverage 57.73% 57.92% +0.18%
==========================================
Files 96 96
Lines 9475 9532 +57
==========================================
+ Hits 5470 5521 +51
- Misses 3544 3546 +2
- Partials 461 465 +4
|
| } | ||
|
|
||
| // delete the DC | ||
| if result := r.deleteDatacenter(ctx, kc, dcName, logger); result != nil { |
There was a problem hiding this comment.
We need to improve naming here, there's a deleteDc and a deleteDatacenter and they do different things.
Maybe deleteDcComponents for the first one, and deleteCassandraDatacenter for the second (matches the name of the CR that it deletes)?
| dcName string, | ||
| remoteClient client.Client) (*stargateapi.Stargate, client.Client, error) { | ||
|
|
||
| func (r *K8ssandraClusterReconciler) findRemoteClientForReaper(ctx context.Context, kcKey client.ObjectKey, dcName string) (client.Client, error) { |
There was a problem hiding this comment.
We know the remote client is going to be the same for all components.
In the original code, as soon as we found a client for Stargate or Reaper, it would get passed to the following methods instead of looking it up again. I think we should preserve that in some form.
In fact we can even improve the lookup, because the current approach is very inefficient. We don't need to look for the components to delete in every context, we already know the context because it's declared in the DC's K8sContext field.
The only problem is that we only have the DC name right now, but that's an easy fix:
- in
checkDcDeletion(), either modifyGetDatacenterForDecommissionso that it returns the fullCassandraDatacenterTemplate, or re-iteratekc.Spec.Cassandra.Datacentersto look it up by name. - then look up the client:
remoteClient, err := r.ClientCache.GetRemoteClient(dcTemplate.K8sContext) if err != nil { ... }
- and finally pass it into
deleteDcand its child methods.
That should allow us to remove all the findRemoteClient methods.
There was a problem hiding this comment.
Ok, I had trouble figuring out this bit, and the old code was confusing. But this makes sense now.
There was a problem hiding this comment.
The only problem is that we only have the DC name right now, but that's an easy fix:
- in
checkDcDeletion(), either modifyGetDatacenterForDecommissionso that it returns the fullCassandraDatacenterTemplate, or re-iteratekc.Spec.Cassandra.Datacentersto look it up by name.
Actually it's not that easy, because the DC has already been removed from kc.Spec.Cassandra.Datacenters by the time we start the cleanup.
| if stargate.Name == stargateName { | ||
| return &stargate, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
(Also applies to the original code) Can't we do a simple lookup by key with remoteClient.Get() instead of this?
We just need the namespace but it should be the same as kcKey if I'm not mistaken.
Pull request was converted to draft
What this PR does:
Refactors cleanup functions to reduce code complexity.
Which issue(s) this PR fixes:
Fixes #961
Checklist