Code review comment for lp://staging/~bjornt/charms/precise/landscape-client/no-landscape-config-call

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