Charm Needed: metis

@marcoceppi: I noticed that it shows Metis when you do a search on jujucharms.com. Sorta confused by the status / change to this "bug" / pull request and was merely following the instructions at https://juju.ubuntu.com/docs/authors-charm-store.html#submitting. Am I following the correct procedure?

The submission was fine, I just cleaned up the wording, assigned it to you, and linked the branch. You will see the charm in the charm store because it's under your personal namespace and not yet promoted. Once it passes review you'll be able to do `juju deploy metis` instead of currently doing `juju deploy cs:~truthfromlies/metis`

You can find our review queue here: https://manage.jujucharms.com/tools/review-queue

Ah, thanks Marco!

Hi Joshua, I'm starting the review on this charm now, I'll get back to you shortly with my feedback

Thanks Marco, keep in mind that the add-relations stuff is skippable, as Metis does not use any form of relations (as of revision 1). Simply pull and deploy! I'll jump in IRC so if you have any questions you can ask there.

Hi Joshua, thanks again for submitting your charm we appreciate your patience while we work through our backlog. Sorry again for the delayed review response, yesterday was a US Holiday so it was a bit distracting.

# Proof

When I run `juju charm proof` (from charm-tools available in ppa:juju/stable) I get the follow results:

> W: Maintainer address should contain a real-name and email only. [Joshua]

We require the format of "Full Name <email address hidden>" for maintainers.

> W: No icon.svg file.

Something we've recently started requiring from charms and charm authors is an icon file which makes our store look more appealing. We provide a template as well as instructions on how to create an icon using Inkscape https://juju.ubuntu.com/docs/authors-charm-icon.html

> W: no copyright file

We require all charms to have a copyright file which provides and licences the charm's content in an OSI approved license. https://juju.ubuntu.com/docs/authors-charm-policy.html and http://opensource.org/licenses

# Review

## metadata.yaml

The charm isn't a subordinate, but it has a relation with a container scope. So essentially that won't actually work. Other than that, see the above remarks about maintainer data

## README

Your README contents are very light on details. You'll want to cover a whole range of topics from deploying, to configuration, to how the charm handles at scale, as well as details on how to file bugs, a bit about the author, etc. You can see a template of the README by running `juju charm add readme`, again available from charm-tools, which will create a boilerplate README.ex file.

## Hooks

You never actually install nginx (or `engine`) in any of the hooks, if this requires a web-service to run I'd assume it would install that web engine. Also, no ports are ever opened using open-port.

### start, stop, restart

These hooks do absolutely nothing, I'd recommend just removing the files all together.

### install

* Nitpick: While you grab the source of the zip over SSL, which is the minimum requirement for cryptographic verification, we recommend you also do payload validation (either by sha1 sumcheck, etc) to validate not only that the payload is what you expected, but it's also whole and intact.

### config-changed

You do the package installation, but never remove the old packages from the previous selection, while it may not be an issue I could see having libapache2-mod-php5 installed when nginx is selected (since apache2 would be installed as a dep) could conflict with port allocation, etc. Also, there is no `set -e` on this file, so if any of the commands failed it likely wouldn't exit the hook properly.

### website-relation-changed

I understand this hasn't been fleshed out yet

# Going forward

Really you're providing a database relation of some sort, likely over either a unique interface like `metis`, or possibly piggybacking on an existing interface that uses the same standard. You'll want to make sure your charm provides that interface so other services can attach and use it.

# Conclusion

This is a great start to the charm! Barring the above mentioned items, I'm sure we'll see it land in the store soon. For now I've m...

"We require the format of "Full Name <email address hidden>" for maintainers."

This has now been resolved.

"Something we've recently started requiring from charms and charm authors is an icon file which makes our store look more appealing."

Hmm, as the system itself has no icon, this is something I need to ponder on. I didn't have it as a high-priority item given a placeholder is used based on the category of the Charm.

"We require all charms to have a copyright file which provides and licences the charm's content in an OSI approved license. https://juju.ubuntu.com/docs/authors-charm-policy.html and http://opensource.org/licenses"

I went ahead and copied the Apache v2 license that is packaged with Metis with the charm itself =)

"You never actually install nginx (or `engine`) in any of the hooks"

Haha wow, you are absolutely right. When you were looking at my Charm install script when I was having issues with it last week, I removed the code that checks for nginx / apache and installs them. This has been re-added.

"Also, no ports are ever opened using open-port."

I didn't feel this was necessary given nginx and apache's start script calls open-port and Metis (given the type of database system) is automatically started when nginx or apache starts.

Other than that, the config-changed will be resolved and an sha1 or md5 hash verification will be added to the Charm (good idea, by the way).

Thanks for reviewing, I'll upload the code to the bzr branch tomorrow and change the status =)

Greetings Joshua,

I would like to confirm that you have the latest revision of your code up? I see a lot of the same issues that Marco Ceppi ran into while starting the review and would like to give you an opportunity to ensure I'm looking at whats current.

I branched: lp:~truthfromlies/charms/precise/metis/trunk/

For now i'm going to mark this bug as incomplete barring your response. When you have validated that it's ready for another review please move the status of this bug to NEW or Fix Committed and I will be happy to progress through the review.

"I see a lot of the same issues that Marco Ceppi ran into while starting the review and would like to give you an opportunity to ensure I'm looking at whats current."

Format (quote - from Marco, my response, quote, etc.):

"We require the format of "Full Name <email address hidden>" for maintainers."

If you check the metadata.yaml file, this is correct: http://bazaar.launchpad.net/~truthfromlies/charms/precise/metis/trunk/view/head:/metadata.yaml
maintainer: "Joshua Strobl <email address hidden>"

"Something we've recently started requiring from charms and charm authors is an icon file which makes our store look more appealing."

An icon exists at http://bazaar.launchpad.net/~truthfromlies/charms/precise/metis/trunk/view/head:/icon.svg

"We require all charms to have a copyright file which provides and licences the charm's content in an OSI approved license. https://juju.ubuntu.com/docs/authors-charm-policy.html and http://opensource.org/licenses"

http://bazaar.launchpad.net/~truthfromlies/charms/precise/metis/trunk/view/head:/copyright shows the Apache License v2

"You never actually install nginx (or `engine`) in any of the hooks"

This is resolved in http://bazaar.launchpad.net/~truthfromlies/charms/precise/metis/trunk/view/head:/hooks/install
- Line 78 and 80

"Your README contents are very light on details. You'll want to cover a whole range of topics from deploying, to configuration, to how the charm handles at scale, as well as details on how to file bugs, a bit about the author, etc."

http://bazaar.launchpad.net/~truthfromlies/charms/precise/metis/trunk/view/head:/README covers requirements, initial configuration, deploying, detailed configuration, exposing Metis, filing bugs / issues, etc.

(Referencing the start, stop, restart scripts): "These hooks do absolutely nothing, I'd recommend just removing the files all together."

You can see in http://bazaar.launchpad.net/~truthfromlies/charms/precise/metis/trunk/files/head:/hooks/ that the start, stop, restart hooks do the necessary resetting, starting, stopping of nginx or apache2.

Referencing the email: "I don't see any of the promised modifications (my biggest tipoff was the missing sha1sum
validation of remote files)"

sha1sum will most likely not be added, at least not anytime soon. Stable builds of Metis will start becoming more frequent, as well as it's dependencies (AtlasUI). As a result, the sha1sum of the zip will change frequently, and unless we are wget'ing the sha1sum from GitHub (which in itself can have some sort of network hiccup and fail), I'd need to have someone evaluate the Juju Charm on a multiple-times per week basis, since the sha1sum would need to be hardcoded or grabbed from a sha1sum file added to the bzr repo. Either way, that means multiple reviews per week, which also means:
a) When a new stable.zip is uploaded to GitHub, any new installs of the Juju Charm will check the sha1sum of the new stable.zip, note that it doesn't match the hard-coded sha1sum, and then fail (since it would be evaluated as a corrupted file).
b) A follow-up on point a, anyone that calls for the Juju Charm to be updated would have the same issues with sha1sum not matching.

Greetings Josh,

I'm going to pin a few notes on the bug report as we had some back and forth over IRC in #Juju

- Cryptographic signature verification for external files are a hard requirement. I feel it would be a good idea for you to implement git as your delivery mechanism since you're considering continuous delivery on your application. I agree that multiple reviews a week will not scale well - so the experience for you *and* your charm users would decline sharply as time goes on and the workload changes.

- it also appears we were pointed at a stale branch somewhere, i don't recall where I was looking but I do see some of the modifications that you have called out specifically in your review.

 I didn't notice any idempotency issues or hard blockers for inclusion in the store.

Thank you again for your patience with the review process as there has been quite a bit of back and forth on the metis charm. Don't forget I've offered to assist in whatever manner you require, just ping me in #juju on irc.freenode.net when you would like to cache in that token.

I'm going to move this back into incomplete pending the delivery of git based file distribution and at such a time the review should go quickly. When you're ready with the requested modifications mark the bug as "Fix Committed" again and someone will be along shortly to review your work.

Hello!

So yea, it's been a while since I've touched this bug and the status will now be set to Fix Committed. The repo is different now, as at the time it was targeted for precise. It is now targeting trusty, although it should work with precise (that remains untested however, I will look into backporting, so don't take my word for it yet).

The necessity for cryptographic signature(s) and automatic file integrity verification is now resolved with the use of git. There was also an unnoticed issue with config-changed where the engine may not have been shut down (apache always threw a fit), this is now resolved as well.

Yea, I'm tired...it's late (well, early, 0700 and haven't gone to bed yet). Probably would be a good idea if you guys had a link to the repo:
https://code.launchpad.net/~truthfromlies/charms/trusty/metis/trunk

Hello Joshua,

Thank you for your continued efforts on the Metis charm. I apologize for the delay since the last review. Our review queue ( http://manage.jujucharms.com/tools/review-queue ) is very large at the moment because so many people are contributing new charms and updates to existing charms. After a charm is reviewed it goes to the end of the queue and starts the journey over again. We appreciate your patience with the review queue process.

The Metis charm is already available in your personal name space at:
http://manage.jujucharms.com/~truthfromlies/precise/metis
http://manage.jujucharms.com/~truthfromlies/trusty/metis
Other people can download and deploy your charm from this personal name space.

To get in the Juju Charm Store charms must be reviewed to ensure they are of the highest quality.

For charms targeted at trusty releases policy is the charm must have amulet tests ( https://juju.ubuntu.com/docs/tools-amulet.html ). Because the Metis charm review started before this became policy this is not a hard requirement but having tests would help ensure quality and help our automated testing effort. An example of some amulet tests are here: http://bazaar.launchpad.net/~charmers/charms/precise/lamp/trunk/view/head:/tests/10-deploy-test.py

# Review

## README

The README file is in markdown format (and looks great by the way!) but will not actually be converted to HTML on the Juju website unless it is saved with the .md extension. After reading the README document I still did not know what Metis was supposed to be doing or how to use Metis. A section on what Metis can be used for and how to access it after deployment would be useful. I set up the nodeList.json and created an “example” directory under /var/www/data/ but I did not know how to proceed. For example do I go to http://metis-ip/Metis/ or something? How do I use or test that Metis is working?

## website-relation-joined

This website-relation-joined hook sets port 8080 for the website relation. After deploying Metis I found the default nginx default web page was available at port 80. This mismatch on ports should be resolved.

Also as was pointed out in an earlier review “open-port” is never called by any of your hooks. If your charm is deployed stand-alone then you need to handle the ports yourself (I did not see instruction on how to use the Metis with other charms such as apache2 or nginx). The command “juju expose” works exclusively with the open-port command. https://juju.ubuntu.com/docs/authors-hook-environment.html#open-port If Metis needs port 80 or 8080 open the hooks must to call open-port. For security reasons the cloud environments default to all ports closed and calling “open-port” works with the security groups of the environment to make the port available.

## install

You use the $USER variable but hard code “root” in the mkdir command on line 63. I would recommend following your $USER convention there.

I would also recommend not using 1777 file permissions for all the Metis files. I believe the convention for nginx and apache is to set the owner of files to www-data.
sudo chown -R www-data:www-data /var/www/Metis
sudo chmod -R 075...

Hey Joshua,

I forgot to mention that running our lint tool "charm proof" against the charm showed one error:
$ charm proof
E: revision file contains non-numeric data

The revision file was used for internal Juju processing many releases ago and was meant to contain a single integer number. The file is now deprecated and no longer needed. I would highly recommend removing the file entirely that would fix this error in charm proof.

Thanks again!