possible synchronizer refactor solution #182
Conversation
Signed-off-by: ianzhang366 <[email protected]>
…test error Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
Signed-off-by: ianzhang366 <[email protected]>
| errLocal := r.Client.Get(context.TODO(), subcfgkey, subitem.SubscriptionConfigMap) | ||
| errRemote := r.hubclient.Get(context.TODO(), subcfgkey, subitem.SubscriptionConfigMap) | ||
|
|
||
| if errRemote != nil && errLocal != nil { |
There was a problem hiding this comment.
Just double checking. Is this purposely an AND condition? Is it ok if just one of them has err?
There was a problem hiding this comment.
here the idea is checking if the reference configmap exists on hub or local. So, if both of them says no, then we really have a problem. Otherwise, we are fine.
There was a problem hiding this comment.
As the obj := obj, the obj is a pointer within the for loop, if we don't do this, then the obj pointer might cause us some issue.
The sonar cloud pointed out this one.
|
|
||
| return nil | ||
| return err |
There was a problem hiding this comment.
Double check please. Always return err if it gets this far?
There was a problem hiding this comment.
you are right. I should've use return nil, which is more friendly. I don't remember why I made this change...
However, using return err is equal to return nil over here. I mean, if the code gets this far, the only way is the err == nil
There was a problem hiding this comment.
Ok you should change it back to return nil in case err is set in the future and code readability.
|
|
||
| pkgMap[dplkey.Name] = true | ||
| //this is for the adaption of the new synchronizer API | ||
| klog.V(10).Infof("ingnor this kvalid %v, only exist for adopting synchronizer API", kvalid) |
There was a problem hiding this comment.
Minor: ingnor should be ignore?
There was a problem hiding this comment.
Hmmm, if we dont have print statement like this, then we have to the kvalid within the function...
However, with the new code changes, we don't actually need to use the kvalid. The idea way is change the function signature to get rid of the kvalid parameter... but I don't want to change the original call chain that much... that's why I did the above print..
There was a problem hiding this comment.
I was referring to your spelling mistake.
| if err != nil { | ||
| err = utils.SetInClusterPackageStatus(&(ghsi.Subscription.Status), dpltosync.GetName(), err, nil) | ||
|
|
||
| ghsi.successful = false |
There was a problem hiding this comment.
minor: put this before the utils.SetInClusterPackageStatus
There was a problem hiding this comment.
I don't get it... why there's a difference? Please explain a bit more
There was a problem hiding this comment.
err = xyz()
if err
Should be together for better code readability.
Also, logic flow wise its better. Setting it to false right away its quick than call the potential longer running function and wait then set.
Signed-off-by: ianzhang366 <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ianzhang366, mikeshng, rokej, xiangjingli 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 |
Signed-off-by: Mike Ng <[email protected]>
A POC PR for the following issue:
https://github.com/open-cluster-management/backlog/issues/1948
Code changes to the following folder, are the one trying to implement the idea, run synchronizer as service, the details of the idea is explained in the above issue,
pkg/synchronizer/kubernetesAlso, with the changes, we don't need the
housekeepingon synchronizer, since the synchronizer will watch on the sync channel(with buffered size as 4 by default)Code changes to the below folder is a small demonstration of the possible changes for the sub-reconcilers code changes, if we use this PR.
pkg/subscriber/namespacePlease let me know your thoughts on this refactor. If you guys like it, I will try to apply the new approach to all sub-reconcilers.
FYI, I haven't fixed the test case, but the namespace subscription flow was tested and succeeded with the following examples,
https://github.com/ianzhang366/acm-applifecycle-samples/tree/master/namespace-ops/distribute-resource-via-namespace