Juju Charms Collection

Bug #1510672
Comment #6

Comment 6 for bug 1510672

Revision history for this message

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

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.

> These settings are required but state they are optional in config.yaml and have no defaults. Please document them as required. Please also use status_set and a status of blocked if any required options are not set.
> access-ports
> vcs-api-version

access-ports and vsc-api-version still have no indication they will error out if not set. Please flesh out the documentation on these entries in config.yaml and the README.

> The following code seems very brittle. Can it be addressed in the source for bambou? Or in your PPA?
> if (config_value_changed('vsdk-packages') and (vsdk_pkgs is not None)):
> source = config('vsdk-repository-url')
> if source is None:
> pip_install(vsdk_pkgs)
> log("Changing the file")
> file_path = "/usr/local/lib/python2.7/dist-packages/" \
> "bambou/nurest_connection.py"
> old_string = "from requests.packages.urllib3.poolmanager" \
> " import PoolManager"
> new_string = "from urllib3.poolmanager import PoolManager"
> change_in_file(file_path, old_string, new_string)
> log("vspk packages installed via pip")

This is going to be a blocker for inclusion into the charm store. This may need to be fixed at the package level.

Related to this. Please add documentation for vsdk-repository-url and make it clear in the README the options for how VSDK gets installed.

> Please make it clear in documentation that the config value vsc-controller-active will be overridden by the relation to vsc.

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.

> Please remove the debug print statements in hooks/vsd_utils.py
> i.e. print "\n\nHello\n"

Still several print statements in this file.

> Please add to the README exactly how the vsrg charm is deployed and related and which config options are required.

Again, it is not clear enough which config parameters are required (and will lead to Exceptions being thrown if not set) for a functional deployment. This needs to be spelled out clearly.

Unit tests are failing
make lint
make test

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.

access-ports and vsc-api-version still have no indication they will error out if not set. Please flesh out the documentation on these entries in config.yaml and the README.

> The following code seems very brittle. Can it be addressed in the source for bambou? Or in your PPA?
>     if (config_value_changed('vsdk-packages') and (vsdk_pkgs is not None)):
>         source = config('vsdk-repository-url')
>         if source is None:
>             pip_install(vsdk_pkgs)
>             log("Changing the file")
>             file_path = "/usr/local/lib/python2.7/dist-packages/" \
>                         "bambou/nurest_connection.py"
>             old_string = "from requests.packages.urllib3.poolmanager" \
>                          " import PoolManager"
>             new_string = "from urllib3.poolmanager import PoolManager"
>             change_in_file(file_path, old_string, new_string)
>             log("vspk packages installed via pip")

This is going to be a blocker for inclusion into the charm store. This may need to be fixed at the package level.

Related to this. Please add documentation for vsdk-repository-url and make it clear in the README the options for how VSDK gets installed.

> Please make it clear in documentation that the config value vsc-controller-active will be overridden by the relation to vsc.

> Please remove the debug print statements in hooks/vsd_utils.py
>     i.e. print "\n\nHello\n"

Still several print statements in this file.

> Please add to the README exactly how the vsrg charm is deployed and related and which config options are required.

Again, it is not clear enough which config parameters are required (and will lead to Exceptions being thrown if not set) for a functional deployment. This needs to be spelled out clearly.

Unit tests are failing
make lint
make test