Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Simplify DNS provider requirements by extracting common parts #228

Closed
1 of 5 tasks
ideahitme opened this issue Jun 7, 2017 · 9 comments
Closed
1 of 5 tasks

Simplify DNS provider requirements by extracting common parts #228

ideahitme opened this issue Jun 7, 2017 · 9 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Milestone
post-v1.0

Comments

@ideahitme
Copy link

ideahitme commented Jun 7, 2017 •
edited
Loading

In the view of few PRs being created to add new dns providers into External DNS it has become more apparent that there are few implicit implementation requirements which can be extracted and made common for all providers:

  • Finding suitable type for the record
  • Filtering records on Records() call based on record type (currently providers only include A, TXT, CNAME records into response)
  • Filtering DNS zones matching domain provided to Provider in the initializer (--domain-filter)
  • Figuring out DNS zone for the record. Currently it matches longest matching hosted zone
  • Dry run - logging potential changes, as the last step substitute for the actual DNS Provider API call

I suggest we do not postpone this until too many providers will need to be refactored

/cc @linki
@ideahitme ideahitme added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 7, 2017
@linki
Copy link
Member

linki commented Jun 7, 2017

I agree.

@ideahitme ideahitme mentioned this issue Jun 8, 2017
Closed
@ideahitme ideahitme mentioned this issue Jun 21, 2017
Closed
@jose5918 jose5918 mentioned this issue Jun 22, 2017
Merged
@linki linki added this to the v0.5 milestone Jun 23, 2017
@ideahitme
Copy link
Author

first improvement (point 3 from above list) was done by @totallyunknown in #252 . Thanks a lot :)

@pigmej pigmej mentioned this issue Aug 1, 2017
Merged
@coreypobrien coreypobrien mentioned this issue Sep 13, 2017
Closed
@njuettner njuettner mentioned this issue Oct 4, 2017
Merged
@coreypobrien
Copy link
Contributor

After working on #356 I think at a minimum AWS needs a unique implementation of Figuring out DNS zone for the record. Currently it matches longest matching hosted zone. Given the complexity of supporting a generic version of filtering provider-specific data structures (e.g. route53.HostedZone), and the need for at least one of them to require a unique implementation, perhaps removing that item from this issue is a good idea?

linki pushed a commit that referenced this issue Oct 11, 2017
@njuettner @linki
#228 Simplify DNS provider requirements by extracting common (#263)
f5639c4 
* #228 Simplify DNS provider requirements by extracting common

* #228 Simplify DNS provider requirements by extracting common

* Missing boilerplate added

* rename recordtypefilter, fail fast statements

* rework zone finder, update deps

* chore: drop namespace from cloudflare method name
@linki linki added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/large labels Jan 2, 2018
ffledgling pushed a commit to ffledgling/external-dns that referenced this issue Jan 18, 2018
@njuettner @linki
kubernetes-sigs#228 Simplify DNS provider requirements by extracting …
d7c27d4 
…common (kubernetes-sigs#263)

* kubernetes-sigs#228 Simplify DNS provider requirements by extracting common

* kubernetes-sigs#228 Simplify DNS provider requirements by extracting common

* Missing boilerplate added

* rename recordtypefilter, fail fast statements

* rework zone finder, update deps

* chore: drop namespace from cloudflare method name
@grahamhayes grahamhayes mentioned this issue Jan 22, 2018
Merged
@ConnorJC3
Copy link

Is this still a blocker for new providers? If so, what needs to be done?

@ideahitme
Copy link
Author

It is not a blocker, more like a wishful thinking. Not having this implemented should not stop us from adding providers.

@linki linki modified the milestones: v0.6, post-v1.0 Jun 14, 2018
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 24, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 24, 2019
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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/test-infra repository.

lou-lan pushed a commit to lou-lan/external-dns that referenced this issue May 11, 2022
@ahmetb
Various fixes to integration tests (kubernetes-sigs#228)
bca4cde 
* Various fixes to integration tests

I fixed a few things about the integration tests that bugged me a litle bit.

- do not actually run the test plugin, it's out of our tool's scope
- run only integration tests from the script (previously ./...)
- use the installed krew when host-krew is present

Signed-off-by: Ahmet Alp Balkan <[email protected]>

* Address code review feedback

Signed-off-by: Ahmet Alp Balkan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
post-v1.0
Development

No branches or pull requests
6 participants
@linki @coreypobrien @ConnorJC3 @k8s-ci-robot @ideahitme @fejta-bot