Creating an OpenMRS Juju Charm

[Matt Bruzek]

This bug is going to track my initial OpenMRS Juju Charm.

Please let me know if you find any issues or have suggestions for improvements.

Thanks!

[Marco Ceppi (marcoceppi)]

Hi Matt! Thanks for starting this bug to track your progress. I've moved the status to Incomplete while you work on this.

Once you have a branch ready for review, please make sure you "Link a related branch" from the right hand side and move the bug status from Incomplete to either "New" or "Fix Committed". That'll place your charm in the review queue and a charmer will be by shortly after to provide a review of the charm.

[Antonio Rosales (arosales)]

Code Branch for Matt's charm is at:

https://code.launchpad.net/~mbruzek-s/charms/precise/openmrs/trunk

-thanks,
Antonio

[Kapil Thangavelu (hazmat)]

quick comments, needs an to open-port for the application so that it can be exposed and accessible publicly. why is this a subordinate charm?

[Matt Bruzek (mbruzek)]

Thanks for the review comments Kapil Thangavelu! I appreciate you taking the time to review my charm.

The reason this is a subordinate charm because it J2EE WAR file that requires a Tomcat server. The normal way to install a web app is to put it on the on the same file system as the Tomcat server. If I had made it as a regular charm it would be on its own system/instance/server and that is not optimal for Java web applications of this kind.

The parent container (tomcat charm) handles the open ports, again since this is a Java web application that installs inside of a tomcat instance. So if the port needs to be changed I would expect that be done on the tomcat charm.

Thanks again for your review. Please let me know if you have any further questions or comments.

[Marco Ceppi (marcoceppi)]

Hi Matt! Thanks for submitting this to the charm store. I'm reviewing it now and will be back with feedback shortly

[Marco Ceppi (marcoceppi)]

Hi again Matt, thank you again for submitting this to the charm store. Below is my initial review of the charm!

# Proof

I ran `juju charm proof`, from charm-tools package, against your charm with the following results:

W: File config.yaml not found.

This warning is a bit misleading, though ideally we'd like to see configuration options for charms but only where they make sense.

# Review

You're downloading directly from sourceforge, but you're not doing any cryptographic verification of the payload (It's not going through SSL, there's not payload validation with either hashes or signatures). This creates a vulnerability in the charm. Since you're hard-coding the version of download, you should be able to create a sha hash of the payload and hard-code that validation portion in the hook. See the Knitpicks section for some additional comments.

In your database-relation-changed hook, you're hardcoding the database name instead of using $db_db. MySQL interface will ensure a database is uniquely created for this service. By hardcoding the name if a user wished to have different openmrs services in the same deployment they'd collide with eachother since they all have the same database name.

# Knitpicks

You're hard-coding a version of the software in the install hook. What we'd prefer to see this as a configuration option. Ideally, you could specify the version of OpenMRS you'd like to use by running `juju set openmrs version=0.9.6` and that would download the appropriate version, validate the payload, and carry on. Alternatively, a user could then upgrade the service when they saw fit by running `juju set openmrs version=0.9.7`. Now, this all depends on how the service can do database schema upgrades, etc.

Thanks again for submitting this charm to the charm store. Let us know if you have any other questions. For the time being I've moved this bug to Incomplete. When you're ready for another review please move the bug status to either New or "Fix Committed" to have it added back in the queue for review.

Thank you again for your effort on this so far, if you have additional questions you can find us in #juju on freenode.net, <email address hidden>, or tag juju on askubuntu.com

[Matt Bruzek (mbruzek)]

Thanks Marco for the through review. All your comments were right on the mark.

I added cryptographic verification on the 1.9.6 version of OpenMRS in the install hook. Thanks for pointing that out to me.

The new code also uses the $db_db variable in the database-relation-changed hook. That was a great spot and I am glad you caught it.

I have tested the changes and everything seems to be working well. Changed the state to Fix Committed please add this back in the queue and review as there is time.

[Marco Ceppi (marcoceppi)]

Hi Matt, I don't see any additional changes to the branch https://code.launchpad.net/~mbruzek-s/charms/precise/openmrs/trunk - please make sure you issue a `bzr push` to have the branch updated. Moving this back to incomplete until the branch is updated.

Move the status back to Fix Committed to have it automatically placed in the Review Queue again once updated! Thanks again for your continued work on this charm.

[Matt Bruzek (mbruzek)]

I have pushed the changes now.

[Marco Ceppi (marcoceppi)]

Reviewing now!

[Marco Ceppi (marcoceppi)]

Hi Matt, sorry I missed this during the initial review, but you've assigned copyright of the code in the charm to Canonical in the copyright file. While we would graciously accept, I don't think it's who you actually intended the copyright of the charm's code to be assigned to. Barring that the charm looks good for inclusion.

There are several improvements that can be made to the charm while I'll file as bugs after promulgation, none are blockers for the store though

[Matt Bruzek (mbruzek)]

Changed the copyright file. Thanks for the review!

[Charles Butler (lazypower)]

Hi Matt,

I've been +1 reviewing this charm and I've run into a blocker. The install hook errors when being run under hte control of juju. When I debug-hooks, and execute hooks/install it returns with a 0 exit code. So i'm unable to track down why this behavior exists, however in my logs in local, it appears that its failing to download the OpenMRS war file from sourceforge.

I will continue to investigate and provide a more thorough review.

[Charles Butler (lazypower)]

Greetings Matt,

per our conversation on IRC the download for OpenMRS 1.9.6 has been replaced due to build errors. Considering you are aware of this change I will place the status of the charm back to "incomplete". When you are ready for another review feel free to change the status to "new" or "needs review" and we will be more than happy to give it another review.

Thank you for your time and continued effort on this charm.

[Matt Bruzek (mbruzek)]

I changed the version to 1.9.7 because version 1.9.6 was removed due to build errors. Also converted the README to markdown format. And fixed up a few things in the hooks. Have another look when you get a chance.

[Charles Butler (lazypower)]

Matt,

Thank you for the continued work on this charm. The setup of OpenMRS was quick and painless thanks to your charm!

Per our conversation, the only nitpick I have on this charm is that the service doesn't auto-correct itself upon removal of the Database, it simply throws an error 500 screen where the application itself could deactivate the container within Tomcat7.

Barring that single change, I don't see anything else that would prevent it from being promulgated into the store.

Thanks again!

[Matt Bruzek (mbruzek)]

Thanks for the review!

It didn't occur to me to remove the database relation after it had been established. I have now tested that case and fixed it. The charm now undeploys OpenMRS when the relation is broken or departed. What is even better is that once the relation is restored all your data is there like it never left!

Thanks for finding that problem. This is once again ready for your review.

[Charles Butler (lazypower)]

+1 LGTM

Thank you for the continued work on this Matt. This charm is up for promulgation - welcome to the charm store my friend.