feat(pdns): support GetDomainFilter interface#6234
feat(pdns): support GetDomainFilter interface#6234clwluvw wants to merge 4 commits intokubernetes-sigs:masterfrom
Conversation
Signed-off-by: Seena Fallah <seenafallah@gmail.com>
|
Hi @clwluvw. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/ok-to-test |
|
Should we remove domainFilter from |
Most likely yes. I'm not too sure about the state of this PR. Adding this What capabilities we are getting? Have you executed a code against a cluster, what is the gain? |
|
Have a look at this PR #6249. It contains contributor guidlines related to provider domainfilters and blueprints |
Signed-off-by: Seena Fallah <seenafallah@gmail.com>
|
Worth to execute against infrastructure, add unit tests, and review if case is missing add unit test(s) here too https://github.com/kubernetes-sigs/external-dns/blob/master/endpoint/domain_filter_test.go Provider manages a.com, b.com. User sets --domain-filter=c.com. With a dynamic GetDomainFilter() the plan sees: Nothing satisfies both - the plan is empty, the controller does nothing. That is the correct and safe outcome. When GetDomainFilter() returns &endpoint.DomainFilter{}, the plan sees: The controller proceeds to reconcile c.com against a provider that may not manage it at all. It's not catastrophic — Records() will return nothing for c.com — but it's a silent fail-open that can cause confusion. |
|
@ivankatliarchuk - Could take another look? i hope i get it right. |
Signed-off-by: Seena Fallah <seenafallah@gmail.com>
a63ccf6 to
8186b49
Compare
Pull Request Test Coverage Report for Build 22730895426Details
💛 - Coveralls |
ivankatliarchuk
left a comment
There was a problem hiding this comment.
Could you as well share results of smoke test, similar to #5085 (comment)
Signed-off-by: Seena Fallah <seenafallah@gmail.com>
@ivankatliarchuk - Is there a reason why we don't have e2e tests in the project for such cases? |
|
Resources infra + plus maintainers time we all just volunteers here. We slowly adding end-2-end tests but for coredns #6162. The project owners decided while back to move providers out of tree, due to maintenance complexity of providers reasons. So at the moment unit test coverage + smoke tests evidences, this is what we rely on. Not ideal, but better then nothing. |
Could you have a look. The code coverage shows new missing lines |
@ivankatliarchuk - Hmm, that is weird. in my coverage report, everything misses are unrelated to the changes here: |
Tested with this script and tests are passing: e2e.sh |
|
How to view your fixtures without downloading them ;-). Sry I do not trust random scripts on internet, and have no sandbox to hand. I'll try to review over weekend once again. |
|
@ivankatliarchuk - Please take a look at https://pastes.io/usrbinenv-52981 |
|
also accessible via https://gist.github.com/clwluvw/33c72331ea33d53cda8befe81dd93a6d |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ivankatliarchuk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I did no know that powerdns could run in kind/minikube. In regards e2e for powerdns it is possible. But most of the provider do require infrastructure and resources. If you want, 100% code coverage, then tutorial similar to #6048 and you could create e2e framework for powerdns, but this will require project owners review. |
What does it do ?
Overrides
GetDomainFilter()for PDNSProvider to return real domain filters. before it was returning empty as of theBaseProviderstruct.Motivation
From #6232 (comment)
More