Merge lp://staging/~rharding/charms/precise/juju-gui/remove-pyjuju into lp://staging/~juju-gui/charms/precise/juju-gui/trunk

Reviewers: mp+201510_code.launchpad.net,

Message:
Please take a look.

Description:
Remove support for PyJuju and rapi from charm.

WIP - couple of question points and failing functional legacy test.

https://code.launchpad.net/~rharding/charms/precise/juju-gui/remove-pyjuju/+merge/201510

(do not edit description out of merge proposal)

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

Affected files (+90, -499 lines):
   M Dependencies.md
   M README.md
   A [revision details]
   M config.yaml
   M config/haproxy.cfg.template
   D config/juju-api-improv.conf.template
   M hooks/backend.py
   M hooks/utils.py
   M tests/20-functional.test
   M tests/test_backends.py
   M tests/test_utils.py

Reviewers: mp+201510_code.launchpad.net,

Message:
Please take a look.

Description:
Remove support for PyJuju and rapi from charm.

WIP - couple of question points and failing functional legacy test.

https://code.launchpad.net/~rharding/charms/precise/juju-gui/remove-pyjuju/+merge/201510

(do not edit description out of merge proposal)

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

Affected files (+92, -695 lines):
   M Dependencies.md
   M Makefile
   M README.md
   A [revision details]
   M config.yaml
   M config/haproxy.cfg.template
   D config/juju-api-agent.conf.template
   D config/juju-api-improv.conf.template
   M hooks/backend.py
   M hooks/utils.py
   M tests/20-functional.test
   M tests/test_backends.py
   M tests/test_utils.py

Reviewers: mp+201510_code.launchpad.net,

Message:
Please take a look.

Description:
Remove support for PyJuju and rapi from charm.

- Removes the support for running rapi/pyjuju.
- Removes the agent used to communicate with zookeeper.
- Removes anything pertaining to zookeeper.
- Attempts to clean up docs and such to make sense with pure juju-core.

Note:

- The functional test run was taking 62min to complete for me. The
timeout is
bumped to 80min to help allow it to complete. This was against ec2.

https://code.launchpad.net/~rharding/charms/precise/juju-gui/remove-pyjuju/+merge/201510

(do not edit description out of merge proposal)

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

Affected files (+92, -695 lines):
   M Dependencies.md
   M Makefile
   M README.md
   A [revision details]
   M config.yaml
   M config/haproxy.cfg.template
   D config/juju-api-agent.conf.template
   D config/juju-api-improv.conf.template
   M hooks/backend.py
   M hooks/utils.py
   M tests/20-functional.test
   M tests/test_backends.py
   M tests/test_utils.py

Thanks a lot for getting rid of all this pyJuju stuff Rick!
This branch is nice: please take a look at my comments below.
LGTM with changes.

https://codereview.appspot.com/51470044/diff/1/README.md
File README.md (right):

https://codereview.appspot.com/51470044/diff/1/README.md#newcode76
README.md:76: core) or "admin" (for pyjuju). The password is the same as
your Juju
We can remove the pyjuju reference here.

https://codereview.appspot.com/51470044/diff/1/README.md#newcode105
README.md:105: For both Juju Core and PyJuju, you must simply do the
following steps. Note
And here too...

https://codereview.appspot.com/51470044/diff/1/README.md#newcode162
README.md:162: #### pyjuju ####
We can get rid of this paragraph and of all the juju-jitsu stuff \o/

https://codereview.appspot.com/51470044/diff/1/config/haproxy.cfg.template
File config/haproxy.cfg.template (right):

https://codereview.appspot.com/51470044/diff/1/config/haproxy.cfg.template#newcode25
config/haproxy.cfg.template:25: # Replace "/ws/" with "/" in any request
path.
Nice.

https://codereview.appspot.com/51470044/diff/1/hooks/backend.py
File hooks/backend.py (left):

https://codereview.appspot.com/51470044/diff/1/hooks/backend.py#oldcode262
hooks/backend.py:262: raise ValueError('Unable to use staging with go
backend')
I am very happy we removed this ValueError for incompatible options.

https://codereview.appspot.com/51470044/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/51470044/diff/1/hooks/utils.py#newcode87
hooks/utils.py:87: Serializer, su,
Please keep "su" in its own line.

https://codereview.appspot.com/51470044/diff/1/hooks/utils.py#newcode148
hooks/utils.py:148: return api_addresses.split()[0]
So you decided to stop supporting also old releases of juju-core.
I am not sure about this: at least we should document it more
explicitly.

https://codereview.appspot.com/51470044/diff/1/hooks/utils.py#newcode320
hooks/utils.py:320: password = password if password else None
Maybe rewrite those three lines like the following?
if not password:
     password = 'admin' if sandbox else None

https://codereview.appspot.com/51470044/diff/1/hooks/utils.py#newcode362
hooks/utils.py:362: # In PyJuju environments, use the same certificate
for both HTTPS and
Please remove this reference to PyJuju.

https://codereview.appspot.com/51470044/diff/1/tests/20-functional.test
File tests/20-functional.test (right):

https://codereview.appspot.com/51470044/diff/1/tests/20-functional.test#newcode45
tests/20-functional.test:45: STAGING_SERVICES = ('haproxy', 'mediawiki',
'memcached', 'mysql', 'wordpress')
STAGING_SERVICES seems to be no longer required.

https://codereview.appspot.com/51470044/diff/1/tests/20-functional.test#newcode46
tests/20-functional.test:46: juju_version().major == 0
Can't we remove this line?
Moreover, can't we remove the juju_version function entirely?

https://codereview.appspot.com/51470044/diff/1/tests/20-functional.test#newcode69
tests/20-functional.test:69: # XXX 2012-11-29 frankban bug=872264:
This XXX comment is no longer required. We can remove all the
cleanup_services machinery.

https://codereview.appspot.com/51470044/diff/1/tests/2...

Reviewers: mp+201510_code.launchpad.net,

Message:
Please take a look.

Description:
Remove support for PyJuju and rapi from charm.

- Removes the support for running rapi/pyjuju.
- Removes the agent used to communicate with zookeeper.
- Removes anything pertaining to zookeeper.
- Attempts to clean up docs and such to make sense with pure juju-core.

Note:

- The functional test run was taking 62min to complete for me. The
timeout is
bumped to 80min to help allow it to complete. This was against ec2.

https://code.launchpad.net/~rharding/charms/precise/juju-gui/remove-pyjuju/+merge/201510

(do not edit description out of merge proposal)

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

Affected files (+107, -847 lines):
   M Dependencies.md
   M Makefile
   M README.md
   A [revision details]
   M config.yaml
   M config/haproxy.cfg.template
   D config/juju-api-agent.conf.template
   D config/juju-api-improv.conf.template
   M hooks/backend.py
   M hooks/utils.py
   M tests/20-functional.test
   M tests/helpers.py
   M tests/test_backends.py
   M tests/test_helpers.py
   M tests/test_utils.py

I'm fighing lbox. When I updated this it put it over at a new url.

https://codereview.appspot.com/52440043

https://codereview.appspot.com/51470044/diff/1/README.md
File README.md (right):

https://codereview.appspot.com/51470044/diff/1/README.md#newcode76
README.md:76: core) or "admin" (for pyjuju). The password is the same as
your Juju
On 2014/01/14 17:26:36, frankban wrote:
> We can remove the pyjuju reference here.

Thanks, removed.

https://codereview.appspot.com/51470044/diff/1/README.md#newcode105
README.md:105: For both Juju Core and PyJuju, you must simply do the
following steps. Note
On 2014/01/14 17:26:36, frankban wrote:
> And here too...

Removed

https://codereview.appspot.com/51470044/diff/1/README.md#newcode162
README.md:162: #### pyjuju ####
On 2014/01/14 17:26:36, frankban wrote:
> We can get rid of this paragraph and of all the juju-jitsu stuff \o/

Removed

https://codereview.appspot.com/51470044/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/51470044/diff/1/hooks/utils.py#newcode87
hooks/utils.py:87: Serializer, su,
On 2014/01/14 17:26:36, frankban wrote:
> Please keep "su" in its own line.

Mistake, thanks for the catch.

https://codereview.appspot.com/51470044/diff/1/hooks/utils.py#newcode148
hooks/utils.py:148: return api_addresses.split()[0]
On 2014/01/14 17:26:36, frankban wrote:
> So you decided to stop supporting also old releases of juju-core.
> I am not sure about this: at least we should document it more
explicitly.

Can you talk about this more? My understanding is that we got rid of the
agent bit and that was the conf file we've removed as part of that. Is
the machiner agent file something different?

https://codereview.appspot.com/51470044/diff/1/hooks/utils.py#newcode320
hooks/utils.py:320: password = password if password else None
On 2014/01/14 17:26:36, frankban wrote:
> Maybe rewrite those three lines like the following?
> if not password:
> password = 'admin' if sandbox else None

Tweaked.

https://codereview.appspot.com/51470044/diff/1/hooks/utils.py#newcode362
hooks/utils.py:362: # In PyJuju environments, use the same certificate
for both HTTPS and
On 2014/01/14 17:26:36, frankban wrote:
> Please remove this reference to PyJuju.

Removed

https://codereview.appspot.com/51470044/diff/1/tests/20-functional.test
File tests/20-functional.test (right):

https://codereview.appspot.com/51470044/diff/1/tests/20-functional.test#newcode45
tests/20-functional.test:45: STAGING_SERVICES = ('haproxy', 'mediawiki',
'memcached', 'mysql', 'wordpress')
On 2014/01/14 17:26:36, frankban wrote:
> STAGING_SERVICES seems to be no longer required.

Wow, how did lint not catch that? Thanks.

https://codereview.appspot.com/51470044/diff/1/tests/20-functional.test#newcode46
tests/20-functional.test:46: juju_version().major == 0
On 2014/01/14 17:26:36, frankban wrote:
> Can't we remove this line?
> Moreover, can't we remove the juju_version function entirely?

Oh hmm, I didn't notice it was only used for pyjuju vs juju-core
distinction. Removing.

https://codereview.appspot.com/51470044/diff/1/tests/20-functional.test#newcode78
tests/20-functional.test:78: if options is None:
On 2014/0...

https://codereview.appspot.com/51470044/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/51470044/diff/1/hooks/utils.py#newcode148
hooks/utils.py:148: return api_addresses.split()[0]
On 2014/01/15 00:03:16, rharding wrote:
> On 2014/01/14 17:26:36, frankban wrote:
> > So you decided to stop supporting also old releases of juju-core.
> > I am not sure about this: at least we should document it more
explicitly.

> Can you talk about this more? My understanding is that we got rid of
the agent
> bit and that was the conf file we've removed as part of that. Is the
machiner
> agent file something different?

Those are two different concepts:
- the pyJuju API agent is the one that was started in the GUI node by
running the rapi-delta branch. The GUI used it as WebSocket server, and
the agent itself was connected to the bootstrap node Zookeper instance.
Your branch get rid of this machinery, and that's ok;
- the machiner agent file is a YAML created by juju-core which includes
the information required by the machiner worker. The API addresses are
part of that information. It must be considered an internal juju-core
detail, and for this reason we introduced the JUJU_API_ADDRESSES env
var, included in the hooks' execution context. But old versions of
juju-core don't support this, so the function strategy was:
   - try to use the environment variable;
   - if it is not present, parse the machiner agent file.
I am comfortable with that strategy, and I'd be inclined to preserve
that behavior. Anyway, as I mentioned, I can be ok in dropping support
for old juju-core releases if we clearly document it.

https://codereview.appspot.com/51470044/