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

[Jelmer Vernooij]

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