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 _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
