Code review comment for lp:~psivaa/uci-engine/django-in-mthood

On 05/11/2014 07:07 AM, Parameswaran Sivatharman wrote:

> === modified file 'charms/precise/python-django/hooks/hooks.py'
> @@ -347,7 +348,6 @@
> remote_host = run("relation-get private-address")
> return remote_host
>
> -
> def get_unit_host():

check out the deletion of that line. I suspect that might cause a pep8
warning. functions need 2 blank lines between them.

> @@ -365,6 +365,15 @@
> with open(destination, 'w') as inject_file:
> inject_file.write(str(template))
>
> +def import_ci_ppa_keys():

ditto on the 2 blank lines

> + cmd = "apt-key adv --keyserver keyserver.ubuntu.com " \
> + "--recv-keys F7F9102D6A8DFC40"
> + try:
> + subprocess.check_call(cmd.split(' '))
> + except:
> + juju_log(MSG_ERROR, "Error importing repo key ")

That's a bit awkward to write a string and split. how about do this:

  def import_ci_ppa_keys():
     cmd = ['apt-key', 'adv', '--keyserver', 'keyserver.ubuntu.com',
            '--recv-keys', 'F7F9102D6A8DFC40']
     try:
         subprocess.check_call(cmd)
     except:
         juju_log(MSG_ERROR, "Error importing repo key ")

Also - what is "F7F9102D6A8DFC40"? Seems like this should probably come
from config.yaml?

You also might name the function "import_ppa_keys" so this isn't totally
tied to ci-airlines (I know it is totally tied to ci-airlines, but lets
not make the problem worse).

> @@ -510,7 +534,8 @@
> else:
> run('git clone %s %s' % (repos_url, vcs_clone_dir))
> elif vcs == 'bzr' or vcs == 'bazaar' or vcs == 'branch':
> - run('bzr branch %s %s' % (repos_url, vcs_clone_dir))
> + run('http_proxy=%s https_proxy=%s bzr branch %s %s'
> + % (proxy_url, proxy_url, repos_url, vcs_clone_dir))

not sure - is this safe when no http_proxy was defined?