Merge lp:~jtv/maas/flatten-dnszoneconfig into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Collapse DNSConfigBase into DNSZoneConfigBase.

Now that DNSConfig is no longer in the family, DNSConfigBase and DNSZoneConfigBase no longer need to be separate classes. This branch combines them, and some of the complexity falls out along the way. To keep refactoring work light, I hard-coded the zone files' access permissions (we never used the property) and to keep a handle on data flows, I moved dns_ip down to DNSForwardZoneConfig (the other classes make no use of it). I believe, but am not entirely certain, that the "mapping" parameter could likewise move down into DNSForwardZoneConfig.

At this point the data flows within the hierarchy become much clearer. They could be shortened a bit further still by passing the mappings and context to the base-class constructor as parameters, instead of having the base class retrieve them by calling overridable methods on itself. Delegation may be more appropriate than a common base class. The constructors for the concrete classes might be a bit easier to grasp if they named their parameters instead of accepting *args and **kwargs. Finally, I suppose shortened_reversed_ip really belongs in DNSReverseZoneConfig.

Jeroen

[Gavin Panella (allenap)]

Looks good. One thing to consider.

[1]

-        networks = kwargs.pop("networks", None)
-        self.networks = [] if networks is None else networks
+        self.networks = kwargs.pop('networks', [])

This subtly changes behaviour. If kwargs["networks"] is None, this will
set self.networks to None. The original would set it to []. I'm not sure
if it's important though.

review: Approve

[Jeroen T. Vermeulen (jtv)]

I know, thanks.

=== modified file 'src/maasserver/dns.py'
--- src/maasserver/dns.py	2014-01-20 07:54:17 +0000
+++ src/maasserver/dns.py	2014-01-21 16:57:51 +0000
@@ -229,11 +229,10 @@
     def _gen_reverse_zones(nodegroups, serial, mappings, networks):
         """Generator of reverse zones, sorted by network."""
         get_domain = lambda nodegroup: nodegroup.name
-        dns_ip = get_dns_server_address()
         reverse_nodegroups = sorted(nodegroups, key=networks.get)
         for nodegroup in reverse_nodegroups:
             yield DNSReverseZoneConfig(
-                get_domain(nodegroup), serial=serial, dns_ip=dns_ip,
+                get_domain(nodegroup), serial=serial,
                 mapping=mappings[nodegroup], network=networks[nodegroup])
 
     def __iter__(self):

=== modified file 'src/provisioningserver/dns/config.py'
--- src/provisioningserver/dns/config.py	2014-01-21 13:21:42 +0000
+++ src/provisioningserver/dns/config.py	2014-01-21 16:57:51 +0000
@@ -23,6 +23,7 @@
 
 from abc import (
     ABCMeta,
+    abstractmethod,
     abstractproperty,
     )
 from contextlib import contextmanager
@@ -231,36 +232,6 @@
             raise
 
 
-class DNSConfigBase:
-    __metaclass__ = ABCMeta
-
-    @abstractproperty
-    def template_path(self):
-        """Return the full path of the template to be used."""
-
-    @abstractproperty
-    def target_path(self):
-        """Return the full path of the target file to be written."""
-
-    @property
-    def template_dir(self):
-        return locate_config(TEMPLATES_DIR)
-
-    @property
-    def access_permissions(self):
-        """The access permissions for the config file."""
-        return 0644
-
-    @property
-    def target_dir(self):
-        return conf.DNS_CONFIG_DIR
-
-    def get_context(self):
-        """Dictionary containing parameters to be included in the
-        parameters used when rendering the template."""
-        return {}
-
-
 class DNSConfig:
     """A DNS configuration file.
 
@@ -316,26 +287,24 @@
     return '.'.join(imap(unicode, significant_octets))
 
 
-class DNSZoneConfigBase(DNSConfigBase):
+class DNSZoneConfigBase:
     """Base class for zone writers."""
 
+    __metaclass__ = ABCMeta
+
     template_file_name = 'zone.template'
 
-    def __init__(self, domain, serial=None, mapping=None, dns_ip=None):
+    def __init__(self, domain, serial=None, mapping=None):
         """
         :param domain: The domain name of the forward zone.
         :param serial: The serial to use in the zone file. This must increment
             on each change.
         :param mapping: A hostname:ip-address mapping for all known hosts in
             the zone.
-        :param dns_ip: The IP address of the DNS server authoritative for this
-            zone.
         """
-        super(DNSZoneConfigBase, self).__init__()
         self.domain = domain
         self.serial = serial
         self.mapping = {} if mapping is None else mapping
-        self.dns_ip = dns_ip
 
     @abstractproperty
     def zone_name(self):
@@ -343,13 +312,17 @@
 
     @property
     def template_path(self):
-        return os.path.join(self.template_dir, self.template_file_name)
+        return locate_config(TEMPLATES_DIR, self.template_file_name)
 
     @property
     def target_path(self):
         """Return the full path of the DNS zone file."""
-        return os.path.join(
-            self.target_dir, 'zone.%s' % self.zone_name)
+        return compose_config_path('zone.%s' % self.zone_name)
+
+    @abstractmethod
+    def get_context(self):
+        """Dictionary containing parameters to be included in the
+        parameters used when rendering the template."""
 
     def write_config(self):
         """Write out this DNS zone file.
@@ -361,8 +334,7 @@
         """
         content = render_dns_template(self.template_path, self.get_context())
         with report_missing_config_dir():
-            incremental_write(
-                content, self.target_path, mode=self.access_permissions)
+            incremental_write(content, self.target_path, mode=0644)
 
 
 class DNSForwardZoneConfig(DNSZoneConfigBase):
@@ -373,9 +345,11 @@
 
         :param networks: The networks that the mapping exists within.
         :type networks: Sequence of :class:`netaddr.IPNetwork`
+        :param dns_ip: The IP address of the DNS server authoritative for this
+            zone.
         """
-        networks = kwargs.pop("networks", None)
-        self.networks = [] if networks is None else networks
+        self.networks = kwargs.pop('networks', [])
+        self.dns_ip = kwargs.pop('dns_ip', None)
         super(DNSForwardZoneConfig, self).__init__(*args, **kwargs)
 
     @property

=== modified file 'src/provisioningserver/dns/tests/test_config.py'
--- src/provisioningserver/dns/tests/test_config.py	2014-01-21 13:13:53 +0000
+++ src/provisioningserver/dns/tests/test_config.py	2014-01-21 16:57:51 +0000
@@ -298,8 +298,8 @@
 
     def test_write_config_DNSConfigDirectoryMissing_if_dir_missing(self):
         dnsconfig = DNSConfig()
-        dir_name = factory.make_name('nonesuch')
-        self.patch(DNSConfig, 'target_dir', dir_name)
+        dir_name = patch_dns_config_path(self)
+        os.rmdir(dir_name)
         self.assertRaises(DNSConfigDirectoryMissing, dnsconfig.write_config)
 
     def test_write_config_errors_if_unexpected_exception(self):
@@ -503,8 +503,7 @@
             )
 
     def test_writes_dns_zone_config(self):
-        target_dir = self.make_dir()
-        self.patch(DNSForwardZoneConfig, 'target_dir', target_dir)
+        target_dir = patch_dns_config_path(self)
         domain = factory.getRandomString()
         hostname = factory.getRandomString()
         network = factory.getRandomNetwork()
@@ -523,8 +522,7 @@
                     ])))
 
     def test_writes_dns_zone_config_with_NS_record(self):
-        target_dir = self.make_dir()
-        self.patch(DNSForwardZoneConfig, 'target_dir', target_dir)
+        target_dir = patch_dns_config_path(self)
         network = factory.getRandomNetwork()
         dns_ip = factory.getRandomIPAddress()
         dns_zone_config = DNSForwardZoneConfig(
@@ -541,7 +539,7 @@
                     ])))
 
     def test_config_file_is_world_readable(self):
-        self.patch(DNSForwardZoneConfig, 'target_dir', self.make_dir())
+        patch_dns_config_path(self)
         network = factory.getRandomNetwork()
         dns_zone_config = DNSForwardZoneConfig(
             factory.getRandomString(), serial=random.randint(1, 100),
@@ -637,13 +635,11 @@
             )
 
     def test_writes_dns_zone_config_with_NS_record(self):
-        target_dir = self.make_dir()
-        self.patch(DNSReverseZoneConfig, 'target_dir', target_dir)
+        target_dir = patch_dns_config_path(self)
         network = factory.getRandomNetwork()
-        dns_ip = factory.getRandomIPAddress()
         dns_zone_config = DNSReverseZoneConfig(
             factory.getRandomString(), serial=random.randint(1, 100),
-            dns_ip=dns_ip, network=network)
+            network=network)
         dns_zone_config.write_config()
         self.assertThat(
             os.path.join(
@@ -652,8 +648,7 @@
                 matcher=Contains('IN  NS  %s.' % dns_zone_config.domain)))
 
     def test_writes_reverse_dns_zone_config(self):
-        target_dir = self.make_dir()
-        self.patch(DNSReverseZoneConfig, 'target_dir', target_dir)
+        target_dir = patch_dns_config_path(self)
         domain = factory.getRandomString()
         network = IPNetwork('192.168.0.1/22')
         dns_zone_config = DNSReverseZoneConfig(
@@ -667,10 +662,9 @@
             FileContains(matcher=expected))
 
     def test_reverse_config_file_is_world_readable(self):
-        self.patch(DNSReverseZoneConfig, 'target_dir', self.make_dir())
+        patch_dns_config_path(self)
         dns_zone_config = DNSReverseZoneConfig(
             factory.getRandomString(), serial=random.randint(1, 100),
-            dns_ip=factory.getRandomIPAddress(),
             network=factory.getRandomNetwork())
         dns_zone_config.write_config()
         filepath = FilePath(dns_zone_config.target_path)