Comment 2 for bug 1446966

Revision history for this message
Cory Johns (johnsca) wrote :

In addition to Kevin's comment, I wanted to note a few things I found on the charm:

* The was_url and im_file_name config options are only used in the install hook, and thus are immutable (i.e., they don't respond to post-install changes to the values using `juju set`). This goes against our best practice recommendations (https://jujucharms.com/docs/1.20/authors-charm-best-practice) and can create a confusing user experience.

* There is a default value provided for was_url but it does not seem to be publicly accessible. The charm should work with the default config values, if provided. Perhaps this can be addressed while addressing Kevin's request to make the files available for testing. If not, however, the default should probably be removed (changed to blank) and a check be added to require the user to set it before attempting the download.

* The charm uses the template (grey) icon.svg. It will need an actual icon.

* The charm defines a website (http) relation but does not implement it. A small website-relation-changed hook should be added to set the host and port for the relation.

* Is it useful to have im_install_path as a config option, rather than being hard-coded in the charm? If so, perhaps an example of when / why you would want to change it could be added.

* Is it useful to have separate options for was_url and im_file_name? It seems like the user experience might be cleaner if there was just a URL option, with the file name included.