Code review comment for lp:~bcsaller/pyjuju/lxc-omega

> [2]
>
> +        self.master_template = LXCContainer(
> +            container_template_name,
> +            origin="ppa",
>
> Shouldn't this be dependent on the source of juju itself?
>
> Please check out the env-origin branch that hopefully Jim is landing tomorrow.
> Should be trivial to hook on it.

I'd like to push this to a later branch, for today this will work with
the ppa and enables local development.

>
> [3]
>
>
> -hostname=`curl http://169.254.169.254/latest/meta-data/local-hostname`
> -
> +hostname=`ip -f inet addr |grep 192.168| grep -v '192\.168\.1\.' | awk '{print $2;}'|cut -f 1 -d '/'`
>
> Please drop the example changes for now. It break the examples in EC2, and consequently our waterfall.
>

Changed.

>
> [4]
>
> I haven't followed carefully, but my gut feeling is that the branch is
> a bit shallow on testing.  Can you please verify if the following logic
> is well covered:
>
> - get_master_template
> - start using get_master_template
> - JUJU_PUBLIC_KEY handling in agent.py
> - etc
>

Ådded tests around _get_container which encapsulates
get_master_template, now _get_master_template and private. This change
marks those as internal and the interface made them a bit simpler to
mock in the tests.

[6,7,9] I've removed the lock file, it was a theoretical issue and
wouldn't prevent manual lxc commands from interfering anyway. Instead
we better namespace the template name and check to see if containers
are built to attempt to avoid other conflicts. This new behavior
should aid in restarts of the agent down the line.

> +        if public_key.startswith("'"):
> +            public_key = public_key[1:-1]
>
> Please strip it rather than assuming the position blindly. This logic
> breaks if e.g. there are spaces at the end of the line.

Done.

>
>
>
> [10]
>
> After you do these changes, can you please ask for another review from
> Kapil.  I'll be traveling tomorrow, but if he's happy with the branch
> I'm +1 on it too.  Thank you!