Juju Charms Collection
landscape-client package

Merge lp://staging/~bjornt/charms/precise/landscape-client/no-landscape-config-call into lp://staging/~charmers/charms/precise/landscape-client/trunk

Proposed by Björn Tillenius on 2013-01-23

Status:	Merged
Approved by:	Mark Mims on 2013-02-08
Approved revision:	26
Merge reported by:	Mark Mims
Merged at revision:	not available
Proposed branch:	lp://staging/~bjornt/charms/precise/landscape-client/no-landscape-config-call
Merge into:	lp://staging/~charmers/charms/precise/landscape-client/trunk
Diff against target:	138 lines (+79/-30) 4 files modified hooks/common.py (+69/-0) hooks/config-changed (+5/-23) hooks/juju-info-relation-joined (+4/-6) revision (+1/-1)
To merge this branch:	bzr merge lp://staging/~bjornt/charms/precise/landscape-client/no-landscape-config-call
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Mark Mims (community)	2013-01-23	Approve on 2013-02-08
Thomas Herve (community)		Approve on 2013-01-23
Review via email: mp+144456@code.staging.launchpad.net

Description of the change

Improve error handling, putting the agent into an error state if something goes wrong
when trying to register.

The charm now uses the landscape-client python library instead of calling the
landcape-config executable, so that we have more control over the registration
process.

Revision history for this message

Thomas Herve (therve) wrote on 2013-01-23:

Looks great! Thanks for starting to factor common code.

[1] config-changed and juju-info-relation-joined share lots of code, it'd be nice to add an additional function for those.

[2] I don't think we should exit_with_error on register error: if the server is out for a bit for example, the charm shouldn't not error out, and just retry to register later on. OTOH, I guess it's a problem if you're missing a configuration parameter. I suspect we want to re-register anyway if change the config later on? IE, instead of having the on_error call, I would stop_client_and_disable_init_script() on config-changed.

Thanks!

review: Approve

lp://staging/~bjornt/charms/precise/landscape-client/no-landscape-config-call updated on 2013-01-24

25. By Björn Tillenius on 2013-01-24: Extract remaining common code.

Revision history for this message

Björn Tillenius (bjornt) wrote on 2013-01-24:

On Wed, Jan 23, 2013 at 04:44:28PM -0000, Thomas Herve wrote:
> Review: Approve
>
> Looks great! Thanks for starting to factor common code.
>
> [1] config-changed and juju-info-relation-joined share lots of code,
> it'd be nice to add an additional function for those.

Sure, the code was indeed similar in the two hooks. I've extracted the
common code so that only once or two function calls remain.

> [2] I don't think we should exit_with_error on register error: if the
> server is out for a bit for example, the charm shouldn't not error out,
> and just retry to register later on. OTOH, I guess it's a problem if
> you're missing a configuration parameter. I suspect we want to
> re-register anyway if change the config later on? IE, instead of having
> the on_error call, I would stop_client_and_disable_init_script() on
> config-changed.

I don't agree here. The server being down is just one of many things can
be wrong, and we can't see any difference between the server being down
and a typo in the URL. If the server is indeed down, the user can run
juju resolved when he has confirmed that everything is alright. I'd
rather do that than to hide errors. If it turns out that the server is
down often, we should reconsider this maybe.

--
Björn Tillenius | https://launchpad.net/~bjornt

lp://staging/~bjornt/charms/precise/landscape-client/no-landscape-config-call updated on 2013-01-30

26. By Björn Tillenius on 2013-01-30: Merge official charm.

Revision history for this message

Mark Mims (mark-mims) wrote on 2013-02-08:

lgtm

review: Approve

Revision history for this message

Mark Mims (mark-mims) wrote on 2013-02-08:

I like extracting logic into common.py.. that's nice, but I personally still prefer that to live in some sort of $CHARM_DIR/lib or $CHARM_DIR/includes dir so that only actual _hooks_ live in $CHARM_DIR/hooks.

Totally think this is a minority view still, but just a thought. :)