-
Notifications
You must be signed in to change notification settings - Fork 883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor ensure work #5660
base: master
Are you sure you want to change the base?
refactor ensure work #5660
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: changzhen <changzhen5@huawei.com>
fac6a38
to
8413ed6
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5660 +/- ##
==========================================
- Coverage 35.91% 35.86% -0.05%
==========================================
Files 648 648
Lines 45079 45095 +16
==========================================
- Hits 16189 16174 -15
- Misses 27645 27675 +30
- Partials 1245 1246 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/cc @RainbowMango |
OK. will do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to understand the ideal, please correct me if not the case.
The benefits include:
- Reduced the number of parameters passed in
ensureWork
. - Split the
ensureWork
into two functions.
type workSyncer struct { | ||
client client.Client | ||
interpreter resourceinterpreter.ResourceInterpreter | ||
overrider overridemanager.OverrideManager | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workSyncer
holds some immutable parameters, mainly used to reduce the number of parameters passed to ensureWork(). Is this the idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's like this.
} | ||
} | ||
// ensureWork ensure Work to be created or updated in the target cluster. | ||
func (s *workSyncer) ensureWork(ctx context.Context, workload *unstructured.Unstructured, binding metav1.Object, targetCluster workv1alpha2.TargetCluster, config extraConfig) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ensureWork
is dedicated to syncing a single work, which is split from the original ensureWork
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, more dependencies on single work generation will be extended in extraConfig
in the future.
switch scope { | ||
case apiextensionsv1.NamespaceScoped: | ||
// workSyncer is design for syncing work objects as expected. | ||
type workSyncer struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it would be more appropriate to have this workSyncer
placed in a separate new Go file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I plan to change the common.go
file name to worksyncer.go
when the review is almost complete. This makes it easier to see which parts have been modified during review.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The
ensureWork
function has some load, and its complexity will continue to increase during ongoing iterations. Therefore, some refactoring has been done on this function.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: