ibm-mq review # Review Thank you for authoring the IBM MQ product into a charm! I can tell that a lot of effort went into this charm. I really appreciate that our tools pass such as `charm proof`. I have made some suggestions in my review to make this charm work even better. # charm proof Running charm proof returns no errors or warnings, that is awesome thank you! # icon.svg We generally try to avoid letters in the icon, but this icon is OK if the letters “MQ” is the only logo for the IBM MQ product. I opened the icon.svg in inkscape and it does not appear to have any layers. I would highly suggest moving the current icon to a back up then generating another template. ''' mv icon.svg first_draft.svg charm add icon inkscape icon.svg (edit the PICTOGRAM layer and the background if required) save the image ``` Then follow the icon documentation: https://jujucharms.com/docs/1.20/authors-charm-icon Put the logo in the “PICTOGRAM” layer for best results. It looks like others had commented on the icon and if you are having trouble using inkscape or the other tools please reach out to me, I would be happy to help you get this right. # copyright The copyright for the charm code looks to be Apache 2.0, that works for us! One thing that is done on other IBM charms is including a license file for the IBM MQ license. This would provide a way for users to read and understand the MQ license that they must accept in order for the charm to deploy and install. Perhaps copy the server/readmes/readme_en_US file into the charm directory, or include all languages in a license directory. # metadata.yaml The metadata looks fine. The maintainer field supports a name before the email address. It might help to understand the maintainer is not a person but a group. Juju Support team or something like that. # hooks/install That is a strange approach to use RPM to install the package, I have never seen that in a charm before! Most charm code uncompresses a tar image, but if this works using rpm is fine (just unconventional). # hooks/config-changed The bash code is clean and easy to understand! Thank you for the work here! # README.md The readme converts to HTML and looks to be correct Markdown syntax. The instructions on how download the software are for a Power only and do not match the name that I found for x86_64. May I suggest generalizing the name to a variable or giving an example for Power and x86_64 (the supported platforms). The steps in the readme are wrong, and should be addressed. The instructions result in `files/archives/server/` directory. Notice “archives” is plural (s) and the additional `server` directory. The config-changed code checks in the `$CHARM_DIR/files/archives/...` directory and does not see the files inside the server directory. The readme does not cover what charms or services can be related to this charm. I was curious about what charms relate to this and how this charm can be used. The readme will be converted to HTML and be the first page users see in the charm store. All of the hyperlinks in the readme the text “here”. When writing documentation it is more clear if you use descriptive text that fit in the sentence. Such as: “More information on IBM MQ is available at the [IBM MQ Version 8.0 documentation](http://www-01.ibm.com/support/knowledgecenter/SSFKSJ_8.0.0/com.ibm.mq.helphome.v80.doc/WelcomePagev8r0.htm)” Or: “...information on procuring the MQ product is available on the [Passport Advantage](http://www-01.ibm.com/software/passportadvantage/) web site”. Consider making the links more descriptive with links like IBM MQ product page, or IBM Knowledge center, when appropriate. The readme clearly states that the charm is not functional until the user accepts the IBM license, but the license is not available in the charm. It is really unfortunate that this charm does not include the binary trial of the IBM MQ product (that expires in 90 days anyway). One of the best features of charms is they make it easier to install software in the cloud. Since the charm will not work until the user accepts the IBM license, it would be much less work for the user if the binary was already in the charm or the charm downloaded the binary. Could we reconsider adding the MQ 90 day trial binary in the charm? I have verified the `accept-ibm-mq-license` configuration value prevents the charm from working, that should satisfy the user accepting the license agreement. I worry if there are extra steps (that no other charm has) this will reduce the likely-hood of people using the IBM MQ charm. # tests The tests are pretty basic just a deploy test with no verification that the process is running or configuration is correct. We would like to see a check for the MQ process running and configured. Please refer to the lamp charm tests for some examples of how to run commands on the unit and check contents of configuration files. http://bazaar.launchpad.net/~charmers/charms/precise/lamp/trunk/view/head:/tests/10-deploy-test.py # deploy My deploy of the charm **failed** because the readme instructions were incorrect. I figuired out the problem only by looking at the `config-changed` hook code, and fixed it. By this time the charm was in an error state that had to be resolved. I also wanted to bring up the user flow of the charm. When I deployed the charm without the files in the correct location the `config-changed` hook failed. Meaning the ibm-mq charm had to be resolved before it would accept any other configuration changed events. Being a charmer I know how to resolve these types of errors, but a regular Juju user may not. Please consider using `exit 0` (instead of `exit 1`) when the license is not accepted as the default deployment behavior will always put the charm in error state. If the charm has extra steps it should be tolerant to user error. # Summary Clearly there is a lot of things working here. I would really like to re-open including the 90 day trial binaries in the charm so the users do not have to go through extra steps to deploy this charm. We would also like to see better testing of the charm. These tests will be run automatically and we want the tests making sure the service is running and configured properly. I am going to move this bug into “In Progress” while you address these issues. Please email me, or contact me on irc.freenode.net in #juju if you have questions or need assistance with the tests or readme. Thanks again for all the work! Please put the bug back in "Fix committed" when you want another review.