[PATCH] samba-tool - make provision check for bind version

[Andrew Bartlett]

On Wed, 2014-01-08 at 06:45 +0100, Jelmer Vernooij wrote:
> On Tue, Jan 07, 2014 at 04:08:17PM +1300, Andrew Bartlett wrote:
> > On Mon, 2014-01-06 at 07:45 +0100, Kai Blin wrote:
> > > On 06/01/14 00:45, Andrew Bartlett wrote:
> > > 
> > > Looks good to me.
> > > 
> > > > Reviewed-by: Andrew Bartlett <abartlet at samba.org>
> > > 
> > > Reviewed-by: Kai Blin <kai at samba.org>
> > > 
> > > Andrew, could you take care of the push, please?
> > 
> > I was testing Ricky's patch, and found a small issue in this patch
> > (samba_upgradedns was missing the extra parameter logger).  Can you
> > re-review this please?
> 
> 
> > @@ -870,7 +871,7 @@ def create_dns_update_list(lp, logger, paths):
> >      setup_file(setup_path("spn_update_list"), paths.spn_update_list, None)
> >  
> >  
> > -def create_named_conf(paths, realm, dnsdomain, dns_backend):
> > +def create_named_conf(paths, realm, dnsdomain, dns_backend, logger):
> >      """Write out a file containing zone statements suitable for inclusion in a
> >      named.conf file (including GSS-TSIG configuration).
> >  
> > @@ -879,8 +880,9 @@ def create_named_conf(paths, realm, dnsdomain, dns_backend):
> >      :param dnsdomain: DNS Domain name
> >      :param dns_backend: DNS backend type
> >      :param keytab_name: File name of DNS keytab file
> > +    :param logger Logger object
> ^ Missing colon after "logger". 
> 
> >      """
> > -
> > +    from samba.provision import ProvisioningError
> ^ Please avoid non-toplevel imports. If this is to avoid a circular import, perhaps
> it makes sense to have a different kind of error object in this file?
> 
> >      if dns_backend == "BIND9_FLATFILE":
> >          setup_file(setup_path("named.conf"), paths.namedconf, {
> >                      "DNSDOMAIN": dnsdomain,
> > @@ -894,9 +896,22 @@ def create_named_conf(paths, realm, dnsdomain, dns_backend):
> >          setup_file(setup_path("named.conf.update"), paths.namedconf_update)
> >  
> >      elif dns_backend == "BIND9_DLZ":
> > +        bind_info = subprocess.Popen(['named -V'], shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, cwd='.').communicate()[0]
> ^ This line looks like it is too long
> 
> > +        bind98 = '#'
> > +        bind99 = '#'
> > +        if bind_info.upper().find('BIND 9.8') != -1:
> > +            bind98 = ''
> > +        elif bind_info.upper().find('BIND 9.9') != -1:
> > +            bind99 = ''
> > +        elif bind_info.upper().find('BIND 9.7') != -1:
> > +            raise ProvisioningError("DLZ option incompatible with BIND 9.7.")
> You might want to use '"BIND 9.8" in bind_info.upper()' here rather tahn find() ?
> I find the former easier to read.
> 
> Looks good otherwise.
> 
> Reviewed-By: Jelmer Vernooij <jelmer at samba.org>

Jelmer,

Attached is the patch with the formatting issues addressed.  I've not
addressed the last comment and the non-toplevel import as we just ran
out of time (Garming has since been deep in loadparm).  I know it's not
ideal, but is this OK?  

I dug this back out after I noticed that Ubuntu now carries a patch to
change which of these is the default, and I know this silly little thing
is a pain point for users, so I would like to get it in.

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba



-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-samba-tool-make-provision-check-for-bind-version.patch
Type: text/x-patch
Size: 4865 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140304/a7199425/attachment.bin>