When writing RFC2317 zones, convert "/" to "-" in the filename.

[sim0nx]

When trying to write RFC2317 zones, of the form e.g. 0/25.10.10.10.in-addr.arpa., octodns-sync fails with:

...
with open(filename, 'w') as fh:
^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: './test/0/25.10.10.10.in-addr.arpa.'

To avoid this, we replace the / with a - in the filename.

[ross]

Good catch and the change makes sense. Main request would be to add a test of the new functionality so that we can make sure that it works as expected now and doesn't get broken in the future.

It'd probably look a lot like

octodns-bind/tests/test_provider_octodns_bind.py

Lines 218 to 246 in dffed12

	@patch('octodns_bind.ZoneFileProvider._serial')
	def test_apply(self, serial_mock):
	serial_mock.side_effect = [424344, 454647, 484950]
	with TemporaryDirectory() as td:
	provider = ZoneFileProvider('target', td.dirname)
	# no root NS
	desired = Zone('unit.tests.', [])
	# populate as a target, shouldn't find anything, file wouldn't even
	# exist
	provider.populate(desired, target=True)
	self.assertEqual(0, len(desired.records))
	cname = Record.new(
	desired,
	'cname',
	{'type': 'CNAME', 'ttl': 42, 'value': 'target.unit.tests.'},
	)
	desired.add_record(cname)
	changes = [Create(cname)]
	plan = Plan(None, desired, changes, True)
	provider._apply(plan)
	with open(join(td.dirname, 'unit.tests.')) as fh:
	self.assertEqual(
	'''$ORIGIN unit.tests.

, just swapping the unit.tests. zone name for something with a /.

[sim0nx]

Good catch and the change makes sense. Main request would be to add a test of the new functionality so that we can make sure that it works as expected now and doesn't get broken in the future.

It'd probably look a lot like

octodns-bind/tests/test_provider_octodns_bind.py

Lines 218 to 246 in dffed12

	@patch('octodns_bind.ZoneFileProvider._serial')
	def test_apply(self, serial_mock):
	serial_mock.side_effect = [424344, 454647, 484950]
	with TemporaryDirectory() as td:
	provider = ZoneFileProvider('target', td.dirname)
	# no root NS
	desired = Zone('unit.tests.', [])
	# populate as a target, shouldn't find anything, file wouldn't even
	# exist
	provider.populate(desired, target=True)
	self.assertEqual(0, len(desired.records))
	cname = Record.new(
	desired,
	'cname',
	{'type': 'CNAME', 'ttl': 42, 'value': 'target.unit.tests.'},
	)
	desired.add_record(cname)
	changes = [Create(cname)]
	plan = Plan(None, desired, changes, True)
	provider._apply(plan)
	with open(join(td.dirname, 'unit.tests.')) as fh:
	self.assertEqual(
	'''$ORIGIN unit.tests.

, just swapping the unit.tests. zone name for something with a /.

Thanks for your feedback.
I added a test-case inspired by test_apply as you suggested.
I had to add an NS record as else it would auto-generate one for the SOA record based on the zone name which is invalid. Same goes for the hostmaster_email.

After asserting that the written output is as expected, I stripped the rest of the test_apply part as that is IMHO not required to test again and was problematic with the NS I added earlier (duplicate records error..).

I am not too familiar with the code base so I hope it is fine like that :-)

[ross]

Thanks