Submitted vdbench initial charm for review

Hi Ameet!

Thanks for submitting this charm for inclusion in the charm store.

Please make sure to review the full charm store inclusion policy so we are on the same page.

https://juju.ubuntu.com/docs/policy.html

##
The following points are blockers for official inclusion:
##
1.
On running charm proof in your charm root, I see this:

E: Charms need a maintainer (See RFC2822) - Name <email>
I: relation website has no hooks

The E: line is a problem, as it means that the charm has an error preventing its inclusion in the charm store. With maintainers, we expect charm authors to be responsive to bug reports and generally continue to update the charm as the software and their own usage of the charm grows. Please add yourself as 'maintainer: ...'
##
2.

The README file is just a copy of the old boilerplate README.ex. Please either remove it, or fill it with content explaining some examples of how to use the charm. Metadata says what vdbench does, but it doesn't really tell us how we can use the charm itself.
##
3.

hooks/install does a wget from downloads.sourceforge.net without any kind of cryptographic verification. This is vulnerable to a DNS cache poison / Man In The Middle Attack. Its not quite enough to just change http: to https in your wget btw, because sourceforge will just redirect you to an insecure mirror. I would recommend that you just distribute the tar in the charm, and upgrade it using the upgrade-charm hook whenever a new version is out. More info on this subject is available here:

https://juju.ubuntu.com/CryptographicSourceVerification
##
4.

hooks/stop is just a boiler plate commented out hook. Remove it or, better, make it stop the things started in start. Also start still has the boilerplate comments at the top.
##
5.

you define a provides: website {interface: http}, but you haven't implemented the appropriate 'joined' hook for this, so it won't actually provide the http interface. Remove the relation or implement the hook, which is usually
as easy as:

#!/bin/sh
relation-set hostname=$(unit-get private-address) port=80

##
The rest of these are not blockers to inclusion, just suggestions:
##
6.

hooks/start - this is not idempotent. Assuming ./vdbench -t will fail if it is already running. You most likely need to have pid files and/or init scripts. An upstart job may be as easy as 4 or 5 lines, and would solve your stop hook problem fairly easily.
##
7.

To future proof the charm, just install default-jre-headless in hooks/install
##

Hi Clint,

Thank you for the suggestions! I've updated a new version to address the issues mentioned:
1 - fixed
2 - fixed
3 - I'm based my edit on your suggestions and Charm http://jujucharms.com/charms/precise/mumble-server/hooks/upgrade-charm. However, I'm open to suggestions.
4 - fixed
5 - fixed

Thanks again for your prompt feedback!

I've performed a review of your changes, please see the following bugs/comments.

Bugs
*metadata.yaml, missing maintainer field - I suspect you've erroneously added this to your README instead. (See https://juju.ubuntu.com/docs/write-charm.html if you need an example).

*hooks/install, vdbench should really be extracted into its own directory. This prevents anything within the vdbench tar from overwriting parts of the charm.

Comments

*You probably want vdbench output to go into a new directory per run to allow the case that somebody wants to rerun vdbench (presumably by redeploying to the same machine). Creating a directory for the current date/time would suffice.

*Be great if you could specify a custom vdbench parameter file to run. Either via config.yaml option(s) or by including something in the charm.

*I'd remove '-x' option from your shell scripts. You don't need to fill the Juju logs with debug output.

*Your copyright refers to non-existant files. You may also want to update the copyright year :)

Please fix the bugs and consider the comments and resubmit.

Thanks for your work!

Bugs fixed.

Also, I've addressed all comments except for the "custom vdbench param file" because I need to think about the settings/implementation a bit more.

I've reviewed your latest commit.

I've made a few minor changes here - lp:~robert-ayres/charms/precise/vdbench/trunk , in particular:

*Extracting vdbench tar using current user for owner+permissions.
*Protecting the vbench/output directory from modification. vdbench chmods this directory to 0777 recursively. Not sure why, but it's technically a local security flaw.
*Corrected the upgrade hook to call the right scripts and not call the install hook.

I've tested on EC2. Deploying units, upgrading units, re-deploying units all works.

If you're happy with my changes, I'd merge my branch to yours and change the status in this bug.

Thanks.

Merged in your edits. Thank you.

Have promulgated your charm.

Thanks for your (and Terry's ;) ) work!