remove provider store and don't initialize each DNS provider #96
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
main.go
Outdated
| log.Fatal(err) | ||
| } | ||
| dnsprovider.Register("google", dnsprovider.NewGoogleProvider(cfg.GoogleProject, cfg.DryRun)) | ||
| dnsprovider.Register("aws", dnsprovider.NewAWSProvider(cfg.DryRun)) |
There was a problem hiding this comment.
Would it not make more sense to only register/initialize the providers which are required?
E.g. the interface would be more like this:
provider, err := dnsprovider.Initialize(cfg.DNSProvider)Disregard if I'm missing some context.
There was a problem hiding this comment.
yes, sounds right. However, you would just move the following lines into a method in the dnsprovider package (I wouldn't consider this a change).
The registration part here is more annoying. It needs provider specific configuration passed in, e.g. the google project. So we still need it this way I believe. We could pass arbitrary config to your proposed Initialize method and then just forward to the picked provider, but I'm not sure if that would be any better.
There was a problem hiding this comment.
yes passing "arbitrary config" to Initialize method is not best approach IMO. Another way, might be to allow store return DNS provider with/without client. But registration should occur via passing raw object. E.g:
dnsprovider.Register("aws", &dnsprovider.AWSProvider{DryRun:DryRun})
...
then store
dnsprovider.LookUpWithClient(cfg.Dnsprovider) //creates client internally based on dns provider
not sure if this is any better
There was a problem hiding this comment.
and dnsprovider.NewAWSProvider() still initializes the client internally
|
@linki maybe not totally related to the PR, but from this it seems to be that the Though I might be missing the point of |
|
Yeah, I see your point. I though it would become easier with this. However, the One thing that this approach supports and the other doesn't is the ability to have a |
|
I would also like to see the providers registering themselve, then this would just become @mikkeloscar's proposed "activation" of the correct one. |
|
@linki by complexity I meant the need to move out the client initialisation phase to a separate method, which does not seem intuitive to me. Ideally, I would want to get a |
|
@ideahitme @mikkeloscar I dropped the complicated store stuff. |
|
maybe u can fix the PR title, because now it is about removing dnsprovider store :) |
linki
changed the title
|
👍 |
|
👍 |
…tes-sigs#96) * fix(dnsprovider): do not always initialize each DNS provider * fix(dnsprovider): fix unnecessary error return value * ref(dnsprovider): drop the store and use a simple switch for lookup
Fixes kubernetes-sigs#81. There's a bunch of odd rules in the parsing of stdin vs args vs --manifest. We might need to simplify it and be more strict. Signed-off-by: Ahmet Alp Balkan <[email protected]>
This just moves the initialization of the DNS provider API client to the
Initializemethod of the respective provider. This way we can register providers and then look them up via flags. We could also seamlessly support multiple providers eventually.