-
Notifications
You must be signed in to change notification settings - Fork 40k
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
Google Cloud DNS dnsprovider - replacement for #25389 #26020
Conversation
I somehow messed up my local git repo - the easiest way to fix it was to create this new PR, which replaces #25389. |
Seeing as this is identical to #25389, reapplying LGTM here. |
@mfanjie FYI |
Fixed boilerplate comment in rrstype.go. Will reapply LGTM. |
google.golang.org/api/dns is missing from the vendor tree. Investigating...
|
Blocked on #26026 |
@nikhiljindal FYI, as you're doing such a marvellous job of shepherding the cluster federation PR's through the pipeline. |
fyi
|
@bprashanth Yes, thanks, that's why it's blocked on #26026 |
OK, dependency issues should be resolved now. Hopefully this will auto-merge as soon as the merge-bot is working again. |
Same problem as #26026
|
@quinton-hoole |
Re-applying LGTM |
##### update the Godeps/LICENSES file accordingly, but it doesn't. | ||
##### Disabling this check for now to unblock adding dependencies to Kubernetes. | ||
|
||
exit 0 |
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 don't see a P0 issue opened for this, but this will quickly bring us out of license compliance, perhaps with this PR itself.
Please open a P0 issue on the license issue and reapply LGTM. |
I have a few questions about this PR I'd like to get answered before we merge, so removed LGTM. Please don't re-apply until then. |
@alex-mohr where are the questions? |
@quinton-hoole I cannot repro, but I suspect I'm not putting this together correctly:
It should be easy enough to remove the jq requirement and bash 4 has been out for 7 years. How was that not updated yet? |
OK, I removed all of the {verify|update}-godep-license.sh related stuff in this PR, as the fixes for that are in #26348 (which should merge shortly). All that's required here now is (I hope) to rebase against HEAD once #26348 is in, run hack/update-godep-license.sh, and commit the results. I'll shepherd that through. |
…fully Tested against both real backend and stubbed backend.
OK @madhusudancs @nikhiljindal I've rebased against the above and regenerated godeps licenses. I think this is good to merge now, once tests pass. |
@quinton-hoole |
GCE e2e build/test passed for commit 2240da1. |
@mfanjie notify me |
All tests have passed. Merge queue is blocked, so manually merging, as this is a v1.3 blocker, with several other blocker PR's waiting for it to merge. |
@mfanjie FYI, merged. |
working on it. |
…nsprovider Automatic merge from submit-queue AWS Route53 dnsprovider Still needs unit tests, and some other cleanup. Review not urgent, but feel free to make a first pass. Only need to look at the last two commits. The prior commits will go in as #26020. This will need to be rebased against #26020 once that merges. It's a bare minimum implementation, only what's required for Ubernetes Federated Services (managing basic A and CNAME records). More functionality (health checks, geolocation etc) can be fairly easily added as required. It also requires github.com/aws/aws-sdk-go/service/route53 to be vendored into godeps, which I haven't managed to do successfully yet (Oh Godep!) cc: @justinsb FYI
Add Google Cloud DNS dnsprovider.