Merge lp://qastaging/~frankban/juju-quickstart/maas-address into lp://qastaging/juju-quickstart

Proposed by Francesco Banconi
Status: Merged
Merged at revision: 103
Proposed branch: lp://qastaging/~frankban/juju-quickstart/maas-address
Merge into: lp://qastaging/juju-quickstart
Diff against target: 1288 lines (+332/-433)
12 files modified
README.rst (+2/-0)
quickstart/__init__.py (+1/-1)
quickstart/app.py (+21/-31)
quickstart/manage.py (+6/-11)
quickstart/settings.py (+3/-0)
quickstart/tests/helpers.py (+11/-0)
quickstart/tests/test_app.py (+63/-183)
quickstart/tests/test_manage.py (+19/-68)
quickstart/tests/test_utils.py (+18/-0)
quickstart/tests/test_watchers.py (+122/-108)
quickstart/utils.py (+14/-0)
quickstart/watchers.py (+52/-31)
To merge this branch: bzr merge lp://qastaging/~frankban/juju-quickstart/maas-address
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+241263@code.qastaging.launchpad.net

Description of the change

Unit address from the machines watcher only

Only use the mega-watcher for machines to retrieve
the Juju GUI unit address.
This change has several consequences:
- it allows us to apply some logic on how the
  right address is chosen. For instance, now
  we try to resolve public hostnames before
  proceeding, and this should fix the cases
  where a cloud dns is not configured on the
  machine running quickstart. This is the case
  of many maas environments;
- it simplifies parsing the mega-watcher changes;
- more importantly, it breaks compatibility
  with very old versions of juju (<1.18), in which
  the mega-watcher for machines did not include
  machine addresses.

For this reason, quickstart now explicitly
drops support for juju < 1.18.1
(1.18.1 is the version on trusty universe).

This also allows for removing some version
checks in the code, including sudo handling when
calling bootstrap on local envs, several special
cases on the watcher side, and other oddities.

For the reasons above, I bumped the quickstart
version up to 1.5.0.

PS: my apologies for the long diff, hope the code
is still easy to follow. Sorry.

Tests: `make check`

QA:
run quickstart as usual, on local and cloud envs,
check it works properly when run again, etc.
this branch has been already successfully QAed in
a maas environment by Adam (Landscape team).

https://codereview.appspot.com/174790043/

To post a comment you must log in.
Revision history for this message
Francesco Banconi (frankban) wrote :

Reviewers: mp+241263_code.launchpad.net,

Message:
Please take a look.

Description:
Unit address from the machines watcher only

Only use the mega-watcher for machines to retrieve
the Juju GUI unit address.
This change has several consequences:
- it allows us to apply some logic on how the
   right address is chosen. For instance, now
   we try to resolve public hostnames before
   proceeding, and this should fix the cases
   where a cloud dns is not configured on the
   machine running quickstart. This is the case
   of many maas environments;
- it simplifies parsing the mega-watcher changes;
- more importantly, it breaks compatibility
   with very old versions of juju (<1.18), in which
   the mega-watcher for machines did not include
   machine addresses.

For this reason, quickstart now explicitly
drops support for juju < 1.18.1
(1.18.1 is the version on trusty universe).

This also allows for removing some version
checks in the code, including sudo handling when
calling bootstrap on local envs, several special
cases on the watcher side, and other oddities.

For the reasons above, I bumped the quickstart
version up to 1.5.0.

PS: my apologies for the long diff, hope the code
is still easy to follow. Sorry.

Tests: `make check`

QA:
run quickstart as usual, on local and cloud envs,
check it works properly when run again, etc.
this branch has been already successfully QAed in
a maas environment by Adam (Landscape team).

https://code.launchpad.net/~frankban/juju-quickstart/maas-address/+merge/241263

(do not edit description out of merge proposal)

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

Affected files (+334, -433 lines):
   M README.rst
   A [revision details]
   M quickstart/__init__.py
   M quickstart/app.py
   M quickstart/manage.py
   M quickstart/settings.py
   M quickstart/tests/helpers.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_manage.py
   M quickstart/tests/test_utils.py
   M quickstart/tests/test_watchers.py
   M quickstart/utils.py
   M quickstart/watchers.py

Revision history for this message
Brad Crittenden (bac) wrote :

On 2014/11/10 12:30:34, frankban wrote:
> Please take a look.

LGTM. Have not done QA yet.

https://codereview.appspot.com/174790043/

Revision history for this message
Brad Crittenden (bac) wrote :
Revision history for this message
Richard Harding (rharding) wrote :

LGTM ty for the updates!

https://codereview.appspot.com/174790043/diff/1/quickstart/app.py
File quickstart/app.py (left):

https://codereview.appspot.com/174790043/diff/1/quickstart/app.py#oldcode188
quickstart/app.py:188: if requires_sudo:
is this because of the new juju requirement?

https://codereview.appspot.com/174790043/

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

Thank you both for the reviews of this long diff!

https://codereview.appspot.com/174790043/diff/1/quickstart/app.py
File quickstart/app.py (left):

https://codereview.appspot.com/174790043/diff/1/quickstart/app.py#oldcode188
quickstart/app.py:188: if requires_sudo:
On 2014/11/10 15:25:20, rharding wrote:
> is this because of the new juju requirement?

Yes! We got rid of very old versions of juju requiring an explicit sudo
on the command line.

https://codereview.appspot.com/174790043/

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

*** Submitted:

Unit address from the machines watcher only

Only use the mega-watcher for machines to retrieve
the Juju GUI unit address.
This change has several consequences:
- it allows us to apply some logic on how the
   right address is chosen. For instance, now
   we try to resolve public hostnames before
   proceeding, and this should fix the cases
   where a cloud dns is not configured on the
   machine running quickstart. This is the case
   of many maas environments;
- it simplifies parsing the mega-watcher changes;
- more importantly, it breaks compatibility
   with very old versions of juju (<1.18), in which
   the mega-watcher for machines did not include
   machine addresses.

For this reason, quickstart now explicitly
drops support for juju < 1.18.1
(1.18.1 is the version on trusty universe).

This also allows for removing some version
checks in the code, including sudo handling when
calling bootstrap on local envs, several special
cases on the watcher side, and other oddities.

For the reasons above, I bumped the quickstart
version up to 1.5.0.

PS: my apologies for the long diff, hope the code
is still easy to follow. Sorry.

Tests: `make check`

QA:
run quickstart as usual, on local and cloud envs,
check it works properly when run again, etc.
this branch has been already successfully QAed in
a maas environment by Adam (Landscape team).

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

https://codereview.appspot.com/174790043/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches