Skip to content

Fix Linode provider#333

Merged
tlimoncelli merged 1 commit intoStackExchange:masterfrom
koesie10:fix-linode
Mar 8, 2018
Merged

Fix Linode provider#333
tlimoncelli merged 1 commit intoStackExchange:masterfrom
koesie10:fix-linode

Conversation

@koesie10
Copy link
Copy Markdown
Collaborator

@koesie10 koesie10 commented Mar 8, 2018

In response to #323.

I've tested it with the Linode provider, and due to commit 19ca760, the order of the integration tests have changed. So, the failing test's number has changed from 25 to 27 in providers.json.

Moreover, the integration test for TXT records does not pass anymore because fixTxt has been removed (de44559#diff-cbc2e007a10f561a01f36d383d88aa80R259), which the provider relied on.

All other integration tests pass.

This fixes all those problems, but I haven't been able to test whether fixTxt breaks other providers.

@captncraig
Copy link
Copy Markdown
Contributor

Feels to me like the linode provider itself should be populating the appropriate fields rather than relying on the core routine to fix it up after the fact.

@koesie10
Copy link
Copy Markdown
Collaborator Author

koesie10 commented Mar 8, 2018

I would agree, but this was the previous behaviour, so multiple providers might rely on this behaviour being there. Therefore, I added it to the core routine.

@tlimoncelli
Copy link
Copy Markdown
Contributor

True, but we decided it was unreasonable to ask every provider to get this right. We created .SetTargetTXT*() so they don't have to worry about the details. Also, if the struct changes, it puts the responsibility on us to fix the Set*/Get* functions. Letting providers do the wrong thing and fixing it later with fixTXT() was a bad decision on my part and I wanted to fix this.

Soon we'll be unexporting .Target and .Name to enforce all of this.

How about something like this? (I haven't tested it)
https://github.com/StackExchange/dnscontrol/tree/tlim_linode

Tom

@koesie10 koesie10 force-pushed the fix-linode branch 2 times, most recently from e8cf687 to ebc8b10 Compare March 8, 2018 17:05
@koesie10
Copy link
Copy Markdown
Collaborator Author

koesie10 commented Mar 8, 2018

Alright, thanks for the draft, I did test it and the two lines commented out were needed due to the way the Linode API works. I've also changed other obvious references to .Target and .Name to use the getters/setters.

@tlimoncelli tlimoncelli merged commit afa80c2 into StackExchange:master Mar 8, 2018
@tlimoncelli
Copy link
Copy Markdown
Contributor

Thanks for the submission!

rblenkinsopp pushed a commit to rblenkinsopp/dnscontrol that referenced this pull request Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants