refactor(controller): reduce cyclomatic complexity of buildProvider#6133
refactor(controller): reduce cyclomatic complexity of buildProvider#6133AndrewCharlesHay wants to merge 10 commits intokubernetes-sigs:masterfrom
Conversation
k8s-ci-robot
added
controller
Pull Request Test Coverage Report for Build 21227118902Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
vflaux
left a comment
vflaux
left a comment
There was a problem hiding this comment.
That’s something I’ve had in mind for a while too, and I’m in favor of this change.
We could then move each providerFactory function to its own package in follow‑up PRs.
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
Co-authored-by: vflaux <38909103+vflaux@users.noreply.github.com>
k8s-ci-robot
added
the
github_actions
| return p, nil | ||
| } | ||
|
|
||
| type providerFactory func(context.Context, *externaldns.Config, *endpoint.DomainFilter) (provider.Provider, error) |
There was a problem hiding this comment.
The benefit is not just in reducting complexity, but standartisation and refactoring. Wdyt to try something like below, this will require some provider standartisation.
type providerFactory map[string]Provider
new providerFactory(string, cfg *externaldns.Config, domainFilter *endpoint.DomainFilter) (provider,error) {
... filters here, as ideally, all providers should support shared filters....
return {
"alibabacloud": alibabacloud.NewAlibabaCloudProvider(...),
"azure": azure.NewAzureProvider(....)
}
}
IF providerFactory is just temporary, we may not event need the struct, hard to say
There was a problem hiding this comment.
Good suggestion. I've implemented the providerFactories map as you proposed.
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v6 | ||
| - name: Set up Go |
There was a problem hiding this comment.
t.Context() was added in Go 1.24. The e2e tests were failing because the Go Version with Ubuntu latest was too old
There was a problem hiding this comment.
Maybe I'm too tired ;-). This e2e script does not require any go installed https://github.com/kubernetes-sigs/external-dns/blob/master/scripts/e2e-test.sh. Could you point where go is required for e2e tests?
There was a problem hiding this comment.
There was a problem hiding this comment.
Ko is a binary that is donwloaded with curl, it does not require go. Unless installed with go install github.com/google/ko@latest command. Could you share where it was failing?
| go handleSigterm(cancel) | ||
|
|
||
| sCfg := source.NewSourceConfig(cfg) | ||
| // TODO: Move source construction to the source package |
There was a problem hiding this comment.
Not sure if this is feasable. This could introduce cyclic dependency.
Moved provider factories to a map in provider_factories.go to address PR kubernetes-sigs#6133 feedback.
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/XL
…akiness" This reverts commit 68e27e3.
|
Not sure, why not to simply move it to provider package? But without this |
|
|
||
| type providerFactory func(context.Context, *externaldns.Config, *endpoint.DomainFilter) (provider.Provider, error) | ||
|
|
||
| var providerFactories = map[string]providerFactory{ |
There was a problem hiding this comment.
Using a map[string]func(...) like this is not inherently "bad Go", but some developers, including self, consider it a form of hidden dependency / service‑locator style factory, which is often indicatory that the code require a redesign.
is effectively:
- A registry keyed by string identifiers.
- Returning an interface (provider.Provider) from factory functions.
- Selected at runtime by name rather than by explicit dependency injection.
Typical criticisms is that that apply directly to a map[string]providerFactory
- Dependencies are discovered only at runtime (string key), not via explicit constructor parameters, which hurts static discoverability and refactoring.
Very much on the edge, as this is not necessary required
var providerFactories = map[string]providerFactory{........logic......}
It's affectively to bypass init(). So it has same issues, as if it was initilized at the init() phase. Sometimes there are no other way, like 3rd library demands it, but in our case, we not forced to do it that way.
I would encourage using common, familiar patterns and avoiding sophisticated machinery when a simpler construct works. This global providerFactories map is an extra layer of indirection that may not buy much over a simple switch. It hard to see what is actually wired where, at least for me.
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Apologies. This was solved slightly different
/close |
|
@ivankatliarchuk: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
5 participants
What does it do ?
This PR refactors the buildProvider function in controller/execute.go to use a map-based lookup (
providerFactories) instead of a largeswitchstatement for initializing DNS providers. This changes the provider instantiation logic from a long series of conditional branches to a direct map lookup. The new complexity of this function is 5.Motivation
The buildProvider function had a high cyclomatic complexity score (37), making it harder to maintain and test. By moving the provider initialization logic into a map of factory functions, we significantly reduce the complexity of the main execution flow and improve code readability.
More