move CreateOrUpdateWork() and related functions to controllers/ctrlutl

[zach593]

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

When I followed up on #6017, I ran into a circular reference issue, move this package to pkg/util could help solve it.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 76.81159% with 16 lines in your changes missing coverage. Please review.

Project coverage is 48.36%. Comparing base (9f8da2c) to head (807153f).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/controllers/ctrlutil/work.go	88.13%	5 Missing and 2 partials ⚠️
pkg/controllers/binding/common.go	0.00%	3 Missing ⚠️
pkg/controllers/mcs/service_export_controller.go	0.00%	1 Missing ⚠️
...clusterservice/endpointslice_collect_controller.go	0.00%	1 Missing ⚠️
...lusterservice/endpointslice_dispatch_controller.go	0.00%	0 Missing and 1 partial ⚠️
.../controllers/multiclusterservice/mcs_controller.go	0.00%	0 Missing and 1 partial ⚠️
...controllers/namespace/namespace_sync_controller.go	0.00%	0 Missing and 1 partial ⚠️
...controllers/unifiedauth/unified_auth_controller.go	0.00%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6018   +/-   ##
=======================================
  Coverage   48.36%   48.36%           
=======================================
  Files         665      666    +1     
  Lines       54831    54831           
=======================================
+ Hits        26520    26521    +1     
+ Misses      26593    26592    -1     
  Partials     1718     1718           

Flag	Coverage Δ
unittests	48.36% <76.81%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[RainbowMango]

/assign

[RainbowMango]

I guess the circular dependency issue is due to the package pkg/util/helper importing its upper-level package pkg/util, which is not right.
Another option would be moving these consts to pkg/util/names, can you help to confirm if it works?

[zach593]

Thank you for your attention @RainbowMango , I'm sorry that I didn't provide the detail about the import chain.

What I met is I want to call prune.RemoveIrrelevantFields() in helper.CreateOrUpdateWork() , but prune.removeJobIrrelevantField() calls helper.ConvertToTypedObject(). so that causes the import cycle.

OTOH, the pkg/helper package contains a lot of code that is very close to the business logic layer. Therefore, even if pkg/helper package's direct dependency on pkg/helper/util is removed, other packages referenced by pkg/helper will still depend on pkg/helper/util . So, removing the direct dependency of the pkg/helper package on pkg/helper/util should have little significance.

[RainbowMango]

What I met is I want to call prune.RemoveIrrelevantFields() in helper.CreateOrUpdateWork() ,
OTOH, the pkg/helper package contains a lot of code that is very close to the business logic layer.

That's the key point. Generally speaking, both pkg/util and pkg/util/helper provide fundamental functions which should be consumed by the business logic layer, and can not refer to business layer.

The function CreateOrUpdateWork you mentioned it's kind of business logic, also part of controllers. Do you think it should be moved out from pkg/util/helpers?

[zach593]

The function CreateOrUpdateWork you mentioned it's kind of business logic, also part of controllers. Do you think it should be moved out from pkg/util/helpers?

I aggre with you, I'll try it and update this PR later.

[zach593]

replace title "move pkg/util/helper/unstructured.go to pkg/util" with "move CreateOrUpdateWork() and related functions to controllers/common"

Looks like it could work, although there is still a lot of business logic in util/helper, but I just moved the necessary parts according to my needs. @RainbowMango

[RainbowMango]

Looks great!

How about naming this new package with the following options?

• ctrlutil
• ctrlhelper
• ctrlcommon

It's no big deal, just feel the common seems to be used everywhere, and I don't like the import aliases.

I prefer the first one, and then the second.

[RainbowMango]

Looks like it could work, although there is still a lot of business logic in util/helper, but I just moved the necessary parts according to my needs. @RainbowMango

Thanks for the reminder, I will be more careful when people try to put more things to util/helper and move things out when they become a blocker.

[zach593]

sure, updated the pkg name.

[RainbowMango]

/lgtm
/approve

/hold
until the ci gets green
Please feel free to unhold this.

[karmada-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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:

• pkg/controllers/OWNERS [RainbowMango]
• pkg/util/OWNERS [RainbowMango]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[RainbowMango]

/hold cancel