Move planner into a new sub-package

samba-in-kubernetes / samba-operator

An operator for a Samba as a service on PVCs in kubernetes

Apache License 2.0

110 stars 24 forks source link

Move planner into a new sub-package #159

Closed phlogistonjohn closed 2 years ago

phlogistonjohn commented 2 years ago

Putting planner into a package has the following benefits (IMO):

The planner type itself can cleanly be broken up over multiple files
Files in the planner package can be group related functionality
Some groups of functions can be additionally group into sub-types
It becomes obvious what functions of the planner are being used by other code, as the those functions must now be public
Newly created private calls can't "accidentally" be used in other code.

Downsides:

Some types funcs are a bit less accessible (needs imports, etc. more typing)
The code churn to make this change

Overall I think its worth it. Also, I wanted to start this refactor before we started messing with the logic we'd need to determine if shares can be combined into a single instance.

spuiuk commented 2 years ago

Just as I was updating the share planner bit to add support for path.

phlogistonjohn commented 2 years ago

I've tried to organize the commits in a way that is fairly reviewable. The approach is to:

copy the original planner.go to a new sub-package
break it up over multiple files
convert those files to public functions and/or public types
swap code in resources pkg to use the new public types/funcs
drop the old file

This is to make more smaller patches but still have each one build cleanly & in theory pass ci checks.

anoopcs9 commented 2 years ago

/retest centos-ci/sink-clustered/mini-k8s-1.23

phlogistonjohn commented 2 years ago

In order not to be rude an prioritize merging my own PRs above others I'm going to let this sit a little while longer. But if still mergable/approved next week I'll work to merge it. So with that said @synarete, @raghavendra-talur, @anoopcs9 last chance for reviews. :-)

synarete commented 2 years ago

In order not to be rude an prioritize merging my own PRs above others I'm going to let this sit a little while longer. But if still mergable/approved next week I'll work to merge it. So with that said @synarete, @raghavendra-talur, @anoopcs9 last chance for reviews. :-)

@phlogistonjohn LGTM, and I would encourage you to merge it into master and let pending PRs rebase.

One minor comment: consider squashing this patch (use 'pl' for Planner): https://github.com/synarete/samba-operator/commit/868d14920f822b68322e887ddee1abd2ceeebc11

phlogistonjohn commented 2 years ago

In order not to be rude an prioritize merging my own PRs above others I'm going to let this sit a little while longer. But if still mergable/approved next week I'll work to merge it. So with that said @synarete, @raghavendra-talur, @anoopcs9 last chance for reviews. :-)

@phlogistonjohn LGTM, and I would encourage you to merge it into master and let pending PRs rebase.

One minor comment: consider squashing this patch (use 'pl' for Planner): synarete@868d149

Thanks! However, I don't think that there's a need to squash it... let's just take that in a follow up PR

phlogistonjohn commented 2 years ago

/retest centos-ci/sink-clustered/mini-k8s-1.21

phlogistonjohn commented 2 years ago

/retest centos-ci/sink-clustered/mini-k8s-1.23