-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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.
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
Create SafeDNS provider #2359
Create SafeDNS provider #2359
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Welcome @rjhenry! |
@rjhenry: What did the ocean say to the beach? Thanks for all the sediment. In 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/test-infra repository. |
/check-cla |
@rjhenry: What did one snowman say to the other snow man? Do you smell carrot? In 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/test-infra repository. |
/check-cla |
/assign raffo |
/assign reviewers |
@rjhenry: GitHub didn't allow me to assign the following users: reviewers. Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In 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/test-infra repository. |
/assign njuettner raffo seanmalloy |
Apologies for the mess of comments on the PR - getting used to the format! Should be in the correct state for Phase 1 - human review. |
docs/tutorials/UKFast_SafeDNS.md
Outdated
|
||
This tutorial describes how to setup ExternalDNS for usage within a Kubernetes cluster using SafeDNS | ||
|
||
Make sure to use **>=FIXME** version of ExternalDNS for this tutorial. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this once I've got a a point of "this PR is reviewed to a technical level", so it can be relatively confident as to which version should be referred to.
The same goes for the two container image tags in examples below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rjhenry can ypu replace the fixme with 0.11.0
? It will be unreleased when we merge this, but it is the next tag for release. Once you change that we're good to merge this one.
@Raffo @seanmalloy @njuettner Is there any idea on when this PR may be able to be looked at? I'm keen to get this in place so I can switch back to the upstream images for ExternalDNS. |
@rjhenry we're incredibly resource constrained this month, so that might take a bit unfortunately. |
@Raffo @seanmalloy @njuettner Now that another month has passed, is there any more likelyhood of being able to review this PR, or are you still incredibly resource-constrained? |
(Note: Rebased from v0.10.0 to v0.10.1, and improved lint quality) |
@rjhenry yes, we’ll take a look in a bit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rjhenry I added a few comments, changes look good overall. I’d be happy to approve this PR once the comments are addressed. Can you please also make sure to add yourself as a maintainer of this code in the README section? We ask for people contributing new provider to provide as well some level of support for the code they will commit.
provider/safedns/safedns.go
Outdated
// TODO: Find a more effient way of doing this. | ||
// Currently iterates over each zoneRecord in ZoneRecords for each Endpoint in UpdateNew; the same will go for | ||
// Delete. As it's double-iteration, that's O(n^2), which isn't great. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be concerned about this? Is it something we are okay to merge like it is and then reconsider in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I haven't noticed any issues running this code in production for an account with many hundred of zones, each with many hundreds of records. On that basis alone, I'd say that it's okay to merge now and reconsider in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sounds good. I would prefer if we leave the TODO then if we do not need any change for now, so that we can search TODOs for real things that are left to do in the codebase.
provider/safedns/safedns.go
Outdated
} | ||
} | ||
for _, endpoint := range changes.Delete { | ||
// TODO: Find a more effient way of doing this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replied above.
I have added my organisation as a maintainer, and not myself as an individual. The organisation has a use for this, but myself as an individual does not, so it makes more sense - in my mind - for the organisation to be deemed the maintainer of this plugin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rjhenry if you drop the TODOs, I’m ok to approve this PR.
I've dropped the TODOs, but kept the comment about the complexity; it's likely to be a good place to look if any performance issues are reported. I've also rebased to be based on v0.10.2 rather than v0.10.1. Many thanks for working with me on this @Raffo! |
@rjhenry I have a comment to solve in https://github.com/kubernetes-sigs/external-dns/pull/2359/files#r778642557 then we can merge this PR. EDIT: there is an issue with the linter, please fix that as well (see CI). |
Keeping the comments in some degree is valuable, as it clarifies the time complexity of the operations done; in the event of a performance issue, this would likely be somewhere to look. However, remove the `TODO` string directly, as it is not an urgent look-at-me-now.
@Raffo I've removed the FIXME (and it's corresponding one in the image name, in the documentation) and fixed the lint issue; is there anything else you need from me here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Raffo, rjhenry The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
Create a provider for UKFast's SafeDNS.
Checklist