Merge into trunk : new-machine-info : Code : juju-quickstart

Status:	Merged
Merged at revision:	62
Proposed branch:	lp://qastaging/~frankban/juju-quickstart/new-machine-info
Merge into:	lp://qastaging/juju-quickstart
Diff against target:	943 lines (+504/-140) 6 files modified Makefile (+2/-1) quickstart/app.py (+19/-7) quickstart/models/envs.py (+3/-3) quickstart/tests/test_app.py (+209/-109) quickstart/tests/test_watchers.py (+179/-11) quickstart/watchers.py (+92/-9)
To merge this branch:	bzr merge lp://qastaging/~frankban/juju-quickstart/new-machine-info
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju GUI Hackers		2014-04-04	Pending
Review via email: mp+214317@code.qastaging.launchpad.net

Revision history for this message

Francesco Banconi (frankban) wrote on 2014-04-04:

#

Reviewers: mp+214317_code.launchpad.net,

Message:
Please take a look.

Description:
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!

https://code.launchpad.net/~frankban/juju-quickstart/new-machine-info/+merge/214317

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/84630043/

Affected files (+500, -139 lines):
   A [revision details]
   M quickstart/app.py
   M quickstart/models/envs.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_watchers.py
   M quickstart/watchers.py

Reviewers: mp+214317_code.launchpad.net,

Message:
Please take a look.

Description:
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!

https://code.launchpad.net/~frankban/juju-quickstart/new-machine-info/+merge/214317

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/84630043/

Affected files (+500, -139 lines):
   A [revision details]
   M quickstart/app.py
   M quickstart/models/envs.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_watchers.py
   M quickstart/watchers.py

Revision history for this message

Brad Crittenden (bac) wrote on 2014-04-04:

#

LGTM. Will QA now.

https://codereview.appspot.com/84630043/diff/1/quickstart/models/envs.py
File quickstart/models/envs.py (right):

https://codereview.appspot.com/84630043/diff/1/quickstart/models/envs.py#newcode444
quickstart/models/envs.py:444: 'The ec2 provider enables you to run Juju
on the EC2 cloud. '
This change looks familiar...

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

https://codereview.appspot.com/84630043/diff/1/quickstart/tests/test_watchers.py#newcode85
quickstart/tests/test_watchers.py:85: # None is returned if an LXC
public address is not available.
Two spaces: LXC public

(I know you hate those!)

https://codereview.appspot.com/84630043/diff/1/quickstart/tests/test_watchers.py#newcode114
quickstart/tests/test_watchers.py:114: # The public address of an LXC
instance is proprely returned.
typo: properly

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

Revision history for this message

Brad Crittenden (bac) wrote on 2014-04-04:

#

QA-OK

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

lp://qastaging/~frankban/juju-quickstart/new-machine-info updated on 2014-04-07

66. By Francesco Banconi on 2014-04-07: Add debug to the Makefile.
67. By Francesco Banconi on 2014-04-07: Changes as per review.

Revision history for this message

Francesco Banconi (frankban) wrote on 2014-04-07:

#

Please take a look.

https://codereview.appspot.com/84630043/diff/1/quickstart/models/envs.py
File quickstart/models/envs.py (right):

https://codereview.appspot.com/84630043/diff/1/quickstart/models/envs.py#newcode444
quickstart/models/envs.py:444: 'The ec2 provider enables you to run Juju
on the EC2 cloud. '
On 2014/04/04 18:24:04, bac wrote:
> This change looks familiar...

Yeah, I really wanted to have these typos fixed in trusty.

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

https://codereview.appspot.com/84630043/diff/1/quickstart/tests/test_watchers.py#newcode85
quickstart/tests/test_watchers.py:85: # None is returned if an LXC
public address is not available.
On 2014/04/04 18:24:04, bac wrote:
> Two spaces: LXC public

> (I know you hate those!)

Indeed! Thank you for spotting this.

https://codereview.appspot.com/84630043/diff/1/quickstart/tests/test_watchers.py#newcode114
quickstart/tests/test_watchers.py:114: # The public address of an LXC
instance is proprely returned.
On 2014/04/04 18:24:04, bac wrote:
> typo: properly

Done.

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

Revision history for this message

Richard Harding (rharding) wrote on 2014-04-07:

#

LGTM, couple of questions/comments. I think I follow the first one now.
If only you could order files in review to line up better.

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 = []
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.

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 = [
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?

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

lp://qastaging/~frankban/juju-quickstart/new-machine-info updated on 2014-04-07

68. By Francesco Banconi on 2014-04-07: Changes as per review.

Revision history for this message

Francesco Banconi (frankban) wrote on 2014-04-07:

#

Download full text (3.5 KiB)

*** 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_watcher...

*** 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/

Revision history for this message

Francesco Banconi (frankban) wrote on 2014-04-07:

#

Thank you both!

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

juju-quickstart

Merge lp://qastaging/~frankban/juju-quickstart/new-machine-info into lp://qastaging/juju-quickstart

Commit message

Description of the change

Preview Diff

Subscribers