Juju Charms Collection

Bug #1452242
Comment #1

Comment 1 for bug 1452242

Revision history for this message

Matt Bruzek (mbruzek) wrote on 2015-05-08:

# Charm ibm-ib review

Hello vsr and thank you for the submission of this new Integration Bus charm! I am very concerned about the extra steps (downloading the binary) creating a burden on the Juju user that other charms do not. Please consider adding an automatic way to get the trial version software and the trial version license to the charm if possible.

Here is my review:

## charm proof
    $ charm proof
    I: all charms should provide at least one thing
    I: missing recommended hook start
    I: missing recommended hook stop

The charm proof tool reports no errors! That is great and reveals a valid charm. Since this software deals with integration I would expect it to provide relations for other charms to consume. The README states that this server must be installed on the same system as IBM MQ, and I see the relation there. Reading the [IBM Knowledge Center](http://www-01.ibm.com/support/knowledgecenter/SSMKHH_10.0.0/com.ibm.etools.mft.doc/bb43020_.htm) tells me this charm supports HTTP, and HTTPS, along with web service protocols. To me that means this charm should also expose the website relation on the “http” interface. Is my assumption invalid here? What other charms can be linked with this charm? How can it be used?

The README instructs the user to download the software and host it on a webserver like apache. This is a big gap in the instructions and a regular Juju user may not know how to host the file. Is the software available to be downloaded automatically? The charm should give more instructions to the user on how to host the file if the software is not available for automatic download.

## config.yaml
There was a misspelling of incorrectly in this file. Since all the variables are tied to this charm, the “ib_” prefix is unnecessary.

## copyright
This looks like a valid Apache 2.0 license. That works for us. Consider adding another license file for the IB software in this directory as well so the user does not have to use the web to find it.

## icon.svg
The icon looks great and is within the parameters.

## revision
This file is no longer used (deprecated) and can safely be deleted.

## install
It is [charm store policy](https://jujucharms.com/docs/devel/authors-charm-policy) that all downloaded software must be cryptographically verified. The install hook does not do this. There are a few ways to fix this problem:

It is always best to include the software in the charm directories. This option is much preferred because files included in the charm directories do not have to be verified. However since the software file is 1.2GB that creates a problem of upload to the cloud. I would really like to see if we can find a way to download the software automatically when deploying the charm.

Is it possible to generate some code that would download the code from the IBM site directly so the user does not have to set up their own HTTP server? Could you do something like this?

The second way is to add a configuration option for the cryptographic sum. The user would then have to set both the URL and the hash sum for the software to install. There are examples of this in other charms as well. Please consider contact me if you need help with this option.

The install hook is where the prerequisites should be installed that are not controlled by configuration. There is a lot of code duplication between install and in config-changed, please see my comments on config-changed.

## config-changed
The mixture of spaces and tabs in this file makes it hard to read in different editors. My suggestion is to use either spaces or tabs but not to mix in the same file.

Both the install and config-changed hooks contain code to download and install the software. The config-changed code should handle all configuration options (which it seems to do correctly) and the install hook should take care of the general needs of the charm that will not change. Make the download logic a function and only call it from config-changed hook. The install hook is for other software dependencies that are not controlled via configuration, such as if you charm needs java installed, or install wget before using it in config-changed.

## tests

The 10-bundles-test.py must be executable if it can be run by our automatic tools. However if the software is not included in the charm directories then the charm will not deploy and the tests are not going to work!

The 10-bundle-test.py file attempts to load a local.yaml file, but the file is not in the code. This seems like an error. When running the file I get `No such file or directory: 'local.yaml'` The tests have to pass unassisted so our automated process can run them. Please make sure the tests run and pass bundletester invoked with the following command:

$ bundletester -F -l DEBUG -v . (when inside the ibm-ib directory)

For more information on tests please check out the precise/lamp tests they check configuration, relations and functionality of the service.

# Summary

This charm needs some more work before it can be recommended. Please consider putting the trial software in the charm along with the appropriate license (as the websphere-liberty charm does) to make it easier for users to use the work you have done. I consider this a big usability issue if they have to take extra steps to download the software to use this charm.

I am going to move the bug in to “In Progress” while you address the issues raised in this review. Please put the bug in “Fix Committed” when you want another review.

# Charm ibm-ib review

Hello vsr and thank you for the submission of this new Integration Bus charm!  I am very concerned about the extra steps (downloading the binary) creating a burden on the Juju user that other charms do not.  Please consider adding an automatic way to get the trial version software and the trial version license to the charm if possible.

Here is my review:

## charm proof 
    $ charm proof
    I: all charms should provide at least one thing
    I: missing recommended hook start
    I: missing recommended hook stop

The charm proof tool reports no errors!  That is great and reveals a valid charm.  Since this software deals with integration I would expect it to provide relations for other charms to consume.  The README states that this server must be installed on the same system as IBM MQ, and I see the relation there.  Reading the [IBM Knowledge Center](http://www-01.ibm.com/support/knowledgecenter/SSMKHH_10.0.0/com.ibm.etools.mft.doc/bb43020_.htm) tells me this charm supports HTTP, and HTTPS, along with web service protocols.  To me that means this charm should also expose the website relation on the “http” interface.  Is my assumption invalid here?  What other charms can be linked with this charm?  How can it be used?

The README instructs the user to download the software and host it on a webserver like apache.  This is a big gap in the instructions and a regular Juju user may not know how to host the file.  Is the software available to be downloaded automatically?  The charm should give more instructions to the user on how to host the file if the software is not available for automatic download.

## config.yaml
There was a misspelling of incorrectly in this file.  Since all the variables are tied to this charm, the “ib_” prefix is unnecessary.

## copyright
This looks like a valid Apache 2.0 license. That works for us.  Consider adding another license file for the IB software in this directory as well so the user does not have to use the web to find it.

## icon.svg
The icon looks great and is within the parameters.

## revision
This file is no longer used (deprecated) and can safely be deleted.

## install
It is [charm store policy](https://jujucharms.com/docs/devel/authors-charm-policy) that all downloaded software must be cryptographically verified.  The install hook does not do this.  There are a few ways to fix this problem:

It is always best to include the software in the charm directories.  This option is much preferred because files included in the charm directories do not have to be verified.  However since the software file is 1.2GB that creates a problem of upload to the cloud.  I would really like to see if we can find a way to download the software automatically when deploying the charm.

Is it possible to generate some code that would download the code from the IBM site directly so the user does not have to set up their own HTTP server?  Could you do something like this?

The second way is to add a configuration option for the cryptographic sum.  The user would then have to set both the URL and the hash sum for the software to install.  There are examples of this in other charms as well.  Please consider contact me if you need help with this option.

## config-changed
The mixture of spaces and tabs in this file makes it hard to read in different editors.  My suggestion is to use either spaces or tabs but not to mix in the same file.

Both the install and config-changed hooks contain code to download and install the software.  The config-changed code should handle all configuration options (which it seems to do correctly) and the install hook should take care of the general needs of the charm that will not change.  Make the download logic a function and only call it from config-changed hook.  The install hook is for other software dependencies that are not controlled via configuration, such as if you charm needs java installed, or install wget before using it in config-changed.

## tests

The 10-bundles-test.py must be executable if it can be run by our automatic tools.  However if the software is not included in the charm directories then the charm will not deploy and the tests are not going to work!

The 10-bundle-test.py file attempts to load a local.yaml file, but the file is not in the code.  This seems like an error.  When running the file I get `No such file or directory: 'local.yaml'`  The tests have to pass unassisted so our automated process can run them.  Please make sure the tests run and pass bundletester invoked with the following command:

$ bundletester -F -l DEBUG -v .  (when inside the ibm-ib directory)

For more information on tests please check out the precise/lamp tests they check configuration, relations and functionality of the service.

# Summary

This charm needs some more work before it can be recommended.  Please consider putting the trial software in the charm along with the appropriate license (as the websphere-liberty charm does) to make it easier for users to use the work you have done.  I consider this a big usability issue if they have to take extra steps to download the software to use this charm.

I am going to move the bug in to “In Progress” while you address the issues raised in this review.  Please put the bug in “Fix Committed” when you want another review.