Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(96)

Issue 174790043: Unit address from the machines watcher only

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 6 months ago by frankban
Modified:
9 years, 6 months ago
Reviewers:
bac, rharding, mp+241263
Visibility:
Public.

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)

Patch Set 1 #

Total comments: 2

Patch Set 2 : Unit address from the machines watcher only #

Unified diffs Side-by-side diffs Delta from patch set Stats (+334 lines, -433 lines) Patch
M README.rst View 1 chunk +2 lines, -0 lines 0 comments Download
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M quickstart/__init__.py View 1 chunk +1 line, -1 line 0 comments Download
M quickstart/app.py View 6 chunks +21 lines, -31 lines 0 comments Download
M quickstart/manage.py View 3 chunks +6 lines, -11 lines 0 comments Download
M quickstart/settings.py View 1 chunk +3 lines, -0 lines 0 comments Download
M quickstart/tests/helpers.py View 1 chunk +11 lines, -0 lines 0 comments Download
M quickstart/tests/test_app.py View 20 chunks +63 lines, -183 lines 0 comments Download
M quickstart/tests/test_manage.py View 4 chunks +19 lines, -68 lines 0 comments Download
M quickstart/tests/test_utils.py View 1 chunk +18 lines, -0 lines 0 comments Download
M quickstart/tests/test_watchers.py View 8 chunks +122 lines, -108 lines 0 comments Download
M quickstart/utils.py View 1 chunk +14 lines, -0 lines 0 comments Download
M quickstart/watchers.py View 7 chunks +52 lines, -31 lines 0 comments Download

Messages

Total messages: 6
frankban
Please take a look.
9 years, 6 months ago (2014-11-10 12:30:34 UTC) #1
bac
On 2014/11/10 12:30:34, frankban wrote: > Please take a look. LGTM. Have not done QA ...
9 years, 6 months ago (2014-11-10 14:31:29 UTC) #2
bac
QA OK
9 years, 6 months ago (2014-11-10 15:23:50 UTC) #3
rharding
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 ...
9 years, 6 months ago (2014-11-10 15:25:20 UTC) #4
frankban
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 ...
9 years, 6 months ago (2014-11-10 15:30:01 UTC) #5
frankban
9 years, 6 months ago (2014-11-10 15:31:59 UTC) #6
*** 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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b