Code review comment for lp:~frankban/juju-quickstart/new-machine-info

Revision history for this message
Francesco Banconi (frankban) wrote :

*** Submitted:

Support MachineInfo addresses.

juju-core 1.18 introduces a change in the mega-watcher:
the MachineInfo includes the public/private addresses
for each machine/container in the environment.
This will be the preferred way to retrieve entity
addresses in future versions of juju-core, which
might also discard the public address field in
the UnitInfo.

This branch updates quickstart so that it can work
in both scenarios: for backward compatibility, the address
is retrieved trying to parse both the unit and the machine
info, without assuming the corresponding fields to be
always included.

This required some testing and documentation efforts,
resulting in a diff longer than usual: sorry about that.

Tests: `make check`.

QA:
Use juju 1.16 (current stable).
In the steps below the command to run is:
`.venv/bin/python juju-quickstart -e {ENV_NAME}`.

1) Bootstrap a local environment with quickstart, ensure
    the quickstart process completes correctly, the juju-gui
    address is retrieved, the GUI is opened. Also ensure
    the user messages showed on stdout make sense.
2) Execute quickstart again, with the local environment already
    bootstrapped. Ensure the process completes correctly,
    and user messages are sane.
3) Destroy the local environment.
4) Bootstrap an ec2 environment with quickstart, ensure
    the quickstart process completes correctly, the juju-gui
    address is retrieved, the GUI is opened. Also ensure
    the user messages showed on stdout make sense.
5) Execute quickstart again, with the ec2 environment already
    bootstrapped. Ensure the process completes correctly,
    and user messages are sane.
6) Destroy the ec2 environment.

Use juju 1.18. This must be compiled from the juju-core 1.18 branch,
which can be found in `lp:juju-core/1.18`.

7) Edit the quickstart/settings.py file included in this branch:
    set `JUJU_CMD` to point to the juju 1.18 path.
8) Follow steps 1) to 6) again, in order to check that
    quickstart works well also with Juju 1.18.

Done, thank you!

R=bac, rharding
CC=
https://codereview.appspot.com/84630043

https://codereview.appspot.com/84630043/diff/20001/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/84630043/diff/20001/quickstart/app.py#newcode420
quickstart/app.py:420: collected_machine_changes = []
On 2014/04/07 12:57:51, rharding wrote:
> Can we use something like a hash keyed by machine_id (or something
else since it
> might not have an id per below) and then just update the changes/state
as it
> comes in to not need to sort/reverse/etc everything. Just always keep
the latest
> info for the key? I'm guessing not, but curious as to the process.

That can be an option, but I preferred to just collect changes rather
than parse them here and update a mutable data structure.
Since the current approach has already been QA'd, I'd be inclined to
preserve this code, at least for this branch.

https://codereview.appspot.com/84630043/diff/20001/quickstart/tests/test_watchers.py
File quickstart/tests/test_watchers.py (right):

https://codereview.appspot.com/84630043/diff/20001/quickstart/tests/test_watchers.py#newcode52
quickstart/tests/test_watchers.py:52: lxc_addresses = [
On 2014/04/07 12:57:51, rharding wrote:
> these can be kvm as well correct? Should these be tested as part of
this or is
> there no difference other than the three chars?

Since there is no support for kvm I can only guess this example also
applies to kvm machines. However yours is a good point, I'll rename this
"container_addresses". Thank you!

https://codereview.appspot.com/84630043/

« Back to merge proposal