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

Jelmer Vernooij jelmer at samba.org
Sat Mar 8 07:04:17 MST 2014


On Tue, Mar 04, 2014 at 11:25:46AM +1300, Andrew Bartlett wrote:
> 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.

Can you at least add a TODO comment above the non-toplevel import, so
others don't blindly copy this?

Other than that, I'm okay with this landing as-is:

Reviewed-By: Jelmer Vernooij <jelmer at samba.org>

Cheers,

jelmer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140308/8dd9009f/attachment.pgp>


More information about the samba-technical mailing list