Fixes #17906 - Move {create,remove}_*_record to dns_common#492
Conversation
|
This also fixes #17879, but doesn't add tests like #489 so I'd prefer to merge that first and then rebase on top of it. |
ekohl
force-pushed
the
17906-add-dns-boilerplate
branch
2 times, most recently
from
4521c8f to
211ecf4
Compare
| Proxy::Dns::Dnscmd::Record.any_instance.expects(:enum_records).with('bar.domain.local', 'host.foo.bar.domain.local', 'A').returns(['1.1.1.1','2.2.2.2']) | ||
| Proxy::Dns::Dnscmd::Record.any_instance.expects(:remove_specific_record_from_zone).with('bar.domain.local', 'host.foo.bar.domain.local', '1.1.1.1', 'A').returns(true) | ||
| Proxy::Dns::Dnscmd::Record.any_instance.expects(:remove_specific_record_from_zone).with('bar.domain.local', 'host.foo.bar.domain.local', '2.2.2.2', 'A').returns(true) | ||
| assert_nil @server.remove_a_record('host.foo.bar.domain.local') |
There was a problem hiding this comment.
I think provider tests can be broken up (which will make them simpler too): the methods that's been pulled into dns_common (create_* and delete_*) can be tested in their own suite. Provider's implementations of do_create and do_delete methods can be tested in their own suites, which will reduce the amount of mocking and make these tests less fragile.
| def test_remove_a_record | ||
| Proxy::Dns::Dnscmd::Record.any_instance.expects(:remove_record).with('host.example.com','A').returns(nil) | ||
| Proxy::Dns::Dnscmd::Record.any_instance.expects(:get_ipv4_address!).with('host.example.com') | ||
| Proxy::Dns::Dnscmd::Record.any_instance.expects(:do_remove).with('host.example.com','A').returns(nil) |
There was a problem hiding this comment.
As an illustration for the above, this and the next three tests should be the same for all providers and can be pulled into a dedicated suite.
| end | ||
|
|
||
| def do_create(name, value, type) | ||
| raise(Proxy::Dns::Error, "Creation of #{type} not implemented") |
There was a problem hiding this comment.
I know this is what we have for do_remove, but is it actually that useful? By default ruby will throw "undefined method" exception, while this will result in a somewhat misleading error message (if you are getting this message do_create isn't implemented at all, not just for a particular record type).
I think having a test that validates that provider implements a required interface might be more helpful (for an example pls. see https://github.com/theforeman/smart-proxy/blob/develop/test/dhcp_isc/dhcp_isc_provider_interface_test.rb)?
There was a problem hiding this comment.
My thinking was that existing providers will have some create functions, but maybe not all. This would maintain compatibility with existing plugins. Maybe we could deprecate overriding the functions and start requiring this in the 1.16 window.
There was a problem hiding this comment.
Sorry, I don't think I explained what I had in mind well: I'm arguing that do_create and do_delete "abstract" methods aren't needed: ruby will throw an exception that's pretty clear when they aren't present, and tests would probably be a better safety net.
There was a problem hiding this comment.
I can see your point, but this way provides a better error message for a plugin which doesn't support CNAMEs. How about do_create and do_remove log a deprecation warning in 1.15 and be removed in 1.16?
ekohl
force-pushed
the
17906-add-dns-boilerplate
branch
from
211ecf4 to
46b999e
Compare
|
Included #489 here because it fixes the tests. |
ekohl
force-pushed
the
17906-add-dns-boilerplate
branch
from
46b999e to
44bea20
Compare
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
Updated. I moved tests around as suggested. This removes the get_* checks in remove_*_record that nsupdate had. I did that since dnscmd didn't had them either (and most likely plugins won't either) and it can mess with caching. That's why I think it's best to remove it for now. This does mean the API changes a bit since no 404 is returned but a 200, but Foreman handles those in the same way. |
|
I should note that I currently don't have access to a working setup to test this, but after review I will make sure this is tested on bind before it's merged. |
theforeman/smart-proxy#492 adds the boiler plate code to dns_common which means we no longer have to include it.
ekohl
changed the title
theforeman/smart-proxy#492 adds the boilerplate to the smart proxy. This reduces the need for it to be copied into each plugin.
|
This looks good. I also verified that current implementation of dns_dnscmd dns provider doesn't raise an exception when an attempt to delete a non-existent record is made -- your changes to remove_* methods standardize this behaviour. |
|
@ekohl: I tested this against bind, seems to be work ok. If you squash and rebase this, I can merge this in today. |
Before most DNS providers had pretty much exact copies of these functions. This removes duplication and allows for much simpler plugins. In doing so it also fixes #17879 by adding CNAME support to the nsupdate module. Lastly it fixes some require statements to allow running single tests rather than relying on other tests to do their requirements for them.
ekohl
force-pushed
the
17906-add-dns-boilerplate
branch
from
44bea20 to
2e626ec
Compare
|
@witlessbird done |
|
Merged. Thanks, @ekohl! |
4 participants
This includes #491 because it makes the diff of the actual patch smaller.