Juju Charms Collection

Bug #1420995
Comment #11

Comment 11 for bug 1420995

Revision history for this message

David Ames (thedac) wrote on 2016-01-25:

#11

Sunny,

Point of clarification, I am looking at the /next branch for this latest review. Please correct me if I should be looking at /trunk. Eventually, /trunk will need to be updated.

> If there is a reason, please document the requirement that these config options be set in the README and in the description field of the config.yaml file.

The README is still not clear enough that the vsc controller will be overridden by the relation. It needs to spelled out when vsc-controller-active is required to be set and when it get overridden by a relation.

In general the README is currently just restating the config.yaml entries. I would still like to see more information in config.yaml in the description for several config options *and* a more descriptive explanation in the README rather than a simple restatement.

It needs to be clear that vrs-repository-url is required in both config.yaml and the README. See also Corry John's point bellow.

Corry John's requests that still need addressing:
> When running your tests, the hard-coded path to "nosetests" caused an issue for me, but I was able to work around it by changing it to use "NOSE := /usr/bin/env nosetests" instead of "PYTHON := /usr/bin/env python". Perhaps you could switch to use that instead?
>
> Also, I noticed that if the install hook is triggered and the vrs-repository-url option is not already set, the charm sets the status to "blocked", but then the handler for ConfigurationError calls sys.exit(1), which will put the service into an error state. In addition to masking your "blocked" status, it will make it so that the admin can't use "juju set" to resolve the issue. Furthermore, by only handling that option in the install hook, you make it so that the option can never be changed (we call this "immutable config", and it is very much frowned upon).
>
> I would recommend moving the handling of that option to the config_changed method (note that config-changed is *always* called after install, at least once) and use your config_value_changed helper to detect changes and handle them appropriately.
>
> One unfortunate downside of unit tests is that you have to be careful to test interactions between the hook handlers themselves. As you mention, your ACTIVE_CONTROLLER roll can be controlled by either the config option or the relation data, but there is some ambiguity as to what happens when there is a conflict between them. In particular, it is possible to have both the vsc-controller-standby config option set and the relation provide a vsc-ip-address value at the same time, in which case the config-changed and relation-changed hooks would fight over the state. This could use some test coverage and explicit resolution to prefer one over the other.

Sunny,

Point of clarification, I am looking at the /next branch for this latest review. Please correct me if I should be looking at /trunk. Eventually, /trunk will need to be updated.

> If there is a reason, please document the requirement that these config options be set in the README and in the description field of the config.yaml file.

It needs to be clear that vrs-repository-url is required in both config.yaml and the README. See also Corry John's point bellow.

Corry John's requests that still need addressing:
> When running your tests, the hard-coded path to "nosetests" caused an issue for me, but I was able to work around it by changing it to use "NOSE := /usr/bin/env nosetests" instead of "PYTHON := /usr/bin/env python". Perhaps you could switch to use that instead?
> 
> Also, I noticed that if the install hook is triggered and the vrs-repository-url option is not already set, the charm sets the status to "blocked", but then the handler for ConfigurationError calls sys.exit(1), which will put the service into an error state. In addition to masking your "blocked" status, it will make it so that the admin can't use "juju set" to resolve the issue. Furthermore, by only handling that option in the install hook, you make it so that the option can never be changed (we call this "immutable config", and it is very much frowned upon).
> 
> I would recommend moving the handling of that option to the config_changed method (note that config-changed is *always* called after install, at least once) and use your config_value_changed helper to detect changes and handle them appropriately.
> 
> One unfortunate downside of unit tests is that you have to be careful to test interactions between the hook handlers themselves. As you mention, your ACTIVE_CONTROLLER roll can be controlled by either the config option or the relation data, but there is some ambiguity as to what happens when there is a conflict between them. In particular, it is possible to have both the vsc-controller-standby config option set and the relation provide a vsc-ip-address value at the same time, in which case the config-changed and relation-changed hooks would fight over the state. This could use some test coverage and explicit resolution to prefer one over the other.