refactor: introduce LookupMember cluster function #855
Conversation
mfrancisc
commented
Aug 24, 2023
mfrancisc
commented
- address comment from feat: Implement SpaceBindingRequest controller main logic #848 (comment)
- extract the member lookup logic to function that is now reused in both spacerequest and sapacebindingrequest controllers.
mfrancisc
requested review from
sbryzak,
xcoulon,
alexeykazakov,
MatousJobanek,
rajivnathan and
ranakan19
pkg/cluster/cluster.go
Outdated
| func LookupMember(MemberClusters map[string]Cluster, request ctrl.Request, object runtimeclient.Object) (Cluster, bool, error) { | ||
| var memberClusterWithObject Cluster | ||
| var err, getError error | ||
| for _, memberCluster := range MemberClusters { | ||
| err = memberCluster.Client.Get(context.TODO(), types.NamespacedName{ | ||
| Namespace: request.Namespace, | ||
| Name: request.Name, | ||
| }, object) | ||
| if err != nil { | ||
| if !errors.IsNotFound(err) { | ||
| // Error reading the object | ||
| getError = err // save the error and return it only later in case the Object was not found on any of the clusters | ||
| } | ||
|
|
||
| // Object not found on current member cluster | ||
| continue | ||
| } | ||
|
|
||
| // save the member cluster on which the Object was found | ||
| memberClusterWithObject = memberCluster | ||
| break // exit once found | ||
| } | ||
|
|
||
| // if we haven't found the Object, the memberCluster is an empty struct, | ||
| // we can't do the same check on the runtimeclient.Object since for some reason it has the TypeMeta field on it | ||
| // and the check fails. | ||
| if reflect.ValueOf(memberClusterWithObject).IsZero() { | ||
| if getError != nil { | ||
| // if we haven't found the Object, and we got an error while trying to retrieve it | ||
| // then let's return the error | ||
| return memberClusterWithObject, false, errs.Wrap(getError, fmt.Sprintf("unable to get the current %T", object)) | ||
| } else if err != nil && errors.IsNotFound(err) { | ||
| // if we exited with a notFound error | ||
| // it means that we couldn't find the Object on any of the given member clusters | ||
| // let's just return false (as not found) and no error | ||
| return memberClusterWithObject, false, nil | ||
| } | ||
| } | ||
| // Object found, return the member cluster that has it | ||
| return memberClusterWithObject, true, nil | ||
| } |
There was a problem hiding this comment.
I tried to simplify the code and also fixed some minor issues. Take a look:
| func LookupMember(MemberClusters map[string]Cluster, request ctrl.Request, object runtimeclient.Object) (Cluster, bool, error) { | |
| var memberClusterWithObject Cluster | |
| var err, getError error | |
| for _, memberCluster := range MemberClusters { | |
| err = memberCluster.Client.Get(context.TODO(), types.NamespacedName{ | |
| Namespace: request.Namespace, | |
| Name: request.Name, | |
| }, object) | |
| if err != nil { | |
| if !errors.IsNotFound(err) { | |
| // Error reading the object | |
| getError = err // save the error and return it only later in case the Object was not found on any of the clusters | |
| } | |
| // Object not found on current member cluster | |
| continue | |
| } | |
| // save the member cluster on which the Object was found | |
| memberClusterWithObject = memberCluster | |
| break // exit once found | |
| } | |
| // if we haven't found the Object, the memberCluster is an empty struct, | |
| // we can't do the same check on the runtimeclient.Object since for some reason it has the TypeMeta field on it | |
| // and the check fails. | |
| if reflect.ValueOf(memberClusterWithObject).IsZero() { | |
| if getError != nil { | |
| // if we haven't found the Object, and we got an error while trying to retrieve it | |
| // then let's return the error | |
| return memberClusterWithObject, false, errs.Wrap(getError, fmt.Sprintf("unable to get the current %T", object)) | |
| } else if err != nil && errors.IsNotFound(err) { | |
| // if we exited with a notFound error | |
| // it means that we couldn't find the Object on any of the given member clusters | |
| // let's just return false (as not found) and no error | |
| return memberClusterWithObject, false, nil | |
| } | |
| } | |
| // Object found, return the member cluster that has it | |
| return memberClusterWithObject, true, nil | |
| } | |
| func LookupMember(memberClusters map[string]Cluster, request ctrl.Request, object runtimeclient.Object) (Cluster, bool, error) { | |
| var memberClusterWithObject Cluster | |
| var getError error | |
| var found bool | |
| for _, memberCluster := range memberClusters { | |
| err := memberCluster.Client.Get(context.TODO(), types.NamespacedName{ | |
| Namespace: request.Namespace, | |
| Name: request.Name, | |
| }, object) | |
| if err != nil { | |
| if !errors.IsNotFound(err) { | |
| // Error reading the object | |
| getError = err // save the error and return it only later in case the Object was not found on any of the clusters | |
| } | |
| // Object not found on current member cluster | |
| continue | |
| } | |
| // save the member cluster on which the Object was found | |
| memberClusterWithObject = memberCluster | |
| found = true | |
| break // exit once found | |
| } | |
| if !found { | |
| if getError != nil { | |
| // if we haven't found the Object, and we got an error while trying to retrieve it | |
| // then let's return the error | |
| return nil, found, errs.Wrap(getError, fmt.Sprintf("unable to get the current %T", object)) | |
| } | |
| // Not found. No Error. | |
| return nil, found, nil | |
| } | |
| // Object found, return the member cluster that has it. Also if we got an error when trying to connec to at least one of them member then return it too. | |
| return memberClusterWithObject, found, getError | |
| } |
There was a problem hiding this comment.
l like having the variable found to observe if the cluster was found, and use that variable in the return statement. It makes it easier to read as well as removes hardcoding the return values. We should return found in lines 64 and 67 as well in the suggested code.
There was a problem hiding this comment.
OK. I updated the suggestion.
There was a problem hiding this comment.
At first I thought about the same, just didn't want to introduce a new variable (found), but if it makes the code easier to read let's keep it.
If we want to return nil instead of memberClusterWithObject I would need to return a pointer and change the signature of all the functions that gets it as input to accept a pointer, I suggest we do this in a separate PR.
plz check e618eac
Thanks for all the suggestions!
There was a problem hiding this comment.
OK. You can keep the memberSluster object as is in the signature. Doesn't have to be a pointer. However please fix the descritpion then. It says it will return nil if not found:
returns nil/false/nil in case the Object was NOT found on any of the given member clusters
returns nil/false/error in case the Object was NOT found on any of the given member clusters...
There was a problem hiding this comment.
Ok thanks for pointing that out, it returns an empty struct and not a nil indeed. I've changed it here: 09cb5e8 I guess this is what you meant.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, mfrancisc, ranakan19 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Kudos, SonarCloud Quality Gate passed!
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #855 +/- ##
==========================================
- Coverage 83.38% 83.33% -0.06%
==========================================
Files 52 52
Lines 5826 5808 -18
==========================================
- Hits 4858 4840 -18
Misses 790 790
Partials 178 178
|
