-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix updates of unchanged records for Azure Private DNS #1377
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 @jasper-d! |
I signed it |
/assign @hjacobs |
Hi Jasper, I verify with a late 2019-version of the master-branch. Regards |
I think we have this behaviour since bf4b176 maybee because of bf4b176#diff-d47c04ae82fc597ab54c2b7998616f1fL109-L110:
|
@saidst Looks like it. I wasn't sure why it worked before, had to look up what TrimLeft actually does :/ |
I will also investigate. |
@saidst Do you think that the Azure API could return something other than I think the only other information that could be used to determine the record type is the actual property used for the targets (i.e. |
I'll debug the concerned piece of code hopefully this afternoon. In any case, I want to complete a fix this week. No matter of which type it is. |
Your changes look good. Thanks! No, Azure's API is OK. The reason is that TrimLeft cannot simply be substituted by TrimPrefix. @linki: This PR shows room for improvement in the unit-tests of this provider but also that late large-scale "code improvements" without practical checks are risky (bf4b176). |
Agree:
Just my 2 cents: How about adding some debug level logging to plan.go? Currently it is kinda a black box. It most certainly works as expected but we don't know the inputs (from logs) and therefore can't tell why it actually behaves the way it does. |
@njuettner, thanks for releasing 0.6.0! This one is really important. Is it feasible to bundle this in a 0.6.1? |
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jasper-d, njuettner 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 |
Due to the missing
/
the record type of existing records had/
as a prefix (i.e./A
). Condition inexternal-dns/plan/plan.go
Lines 236 to 238 in f400ded
Fixes #1373