[Freeipa-devel] [PATCH] 8 Add DNS service records for Windows

[Alexander Bokovoy]

On Fri, 14 Oct 2011, Sumit Bose wrote:
> On Fri, Oct 14, 2011 at 12:15:57PM +0200, Sumit Bose wrote:
> > Hi,
> > 
> > this patch adds DNS service records for for Windows systems during the
> > setup of trust support.
> > 
> > Fixes https://fedorahosted.org/freeipa/ticket/1939.
> > 
> > bye,
> > Sumit
> 
> Alexander made some comments on irc which I tried to integrate into this
> new version of the patch.
Looks good now. I haven't tested it in real life and there are very 
few minor comments bellow.

> +        err_msg = None
> +        ret = api.Command.dns_is_enabled()
> +        if not ret['result']:
> +            err_msg = "DNS is not enabled in this IPA server."
Maybe "DNS management was not enabled at install time."?

> +        else:
> +            if not dns_zone_exists(zone):
> +                err_msg = "The DNS zone %s is not managed on this IPA server" % \
> +                          zone
Same here -- "DNS zone %s cannot be managed as it is not defined in IPA"?

>      def setup(self, fqdn, ip_address, realm_name, domain_name, netbios_name,
> -              smbd_user="samba"):
> +              no_msdcs, smbd_user="samba"):
Maybe we could make no_msdcs defaulting to False here? I.e. 
+                no_msdcs=False, smbd_user="samba"):

It would make more clear what is the default and that it is really 
optional setting -- I'm thinking from the perspective of maintenance 
of the code in future.

-- 
/ Alexander Bokovoy