launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

> === modified file 'src/provisioningserver/omshell.py'
> --- src/provisioningserver/omshell.py   2012-08-07 10:20:38 +0000
> +++ src/provisioningserver/omshell.py   2012-08-22 13:35:57 +0000
> @@ -32,8 +32,12 @@
>
>
>  def call_dnssec_keygen(tmpdir):
> +    dnssec_keygen = 'dnssec-keygen'
> +    if os.path.exists('/usr/sbin/dnssec-keygen'):
> +        dnssec_keygen = '/usr/sbin/dnssec-keygen'
> +
>      return check_output(
> -        ['dnssec-keygen', '-r', '/dev/urandom', '-a', 'HMAC-MD5',
> +        [dnssec_keygen, '-r', '/dev/urandom', '-a', 'HMAC-MD5',
>           '-b', '512', '-n', 'HOST', '-K', tmpdir, '-q', 'omapi_key'])

This is okay, but can I suggest an alternative approach?

I think it would be better to run the command in an environment with a
modified PATH, where /usr/sbin is appended. That way, if the caller
wants to use a different dnssec-keygen they can. The attached diff
demonstrates what I mean.

Another alternative would be to always use /usr/sbin/dnssec-keygen. If
they don't have it, they need to install it.

 +1

-- 
https://code.launchpad.net/~andreserl/maas/maas_lp1040047/+merge/120769
Your team MAAS Maintainers is requested to review the proposed merge of lp:~andreserl/maas/maas_lp1040047 into lp:maas.

Attachment: alt.diff
Description: Binary data