VIVO charm

Bug #996576 reported by Nicholas Skaggs on 2012-05-08
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Juju Charms Collection
Undecided
Nicholas Skaggs

Bug Description

New charm intended for inclusion into charm store, please review.

~nskaggs/charms/precise/vivo/

VIVO enables the discovery of researchers across institutions. Participants in the network include institutions with local installations of VIVO or those with research discovery and profiling applications that can provide semantic web!-compliant data. The information accessible through VIVO's search and browse capability will reside and be controlled locally, within institutional VIVOs or other semantic web-compliant applications.

Changed in charms:
assignee: nobody → Clint Byrum (clint-fewbar)
Juan L. Negron (negronjl) wrote :

Some feedback:

The charm depends on wget which is not installed in lxc containers . To make sure that the charm works regardless of where it is installed, it would be a good idea to explicitly install wget.

-Juan

Changed in charms:
status: New → Incomplete
Clint Byrum (clint-fewbar) wrote :

Hi Nicholas, thanks for submitting a charm!

VIVO looks really cool, I deployed and it seemed to work, though it complained about not having SOLR (dunno if you know, but SOLR *was* just promulgated into the store. :)

== Blocking Changes ==

* You need to cryptographically verify the download from sourceforge. Currently you are trusting that sourceforge.net has not been compromised by a man in the middle.

Since you are just linking directly to v1.4.1, I suggest just putting the tarball in the charm itself. This means you will need to update the charm whenever there are new upstream releases, but you are already in that boat.

* install is not idempotent - install is only run once per deploy, but *sometimes* install has errors, and will have to be retried with 'juju resolved --retry vivo-service-name/0'. Any steps that are not repeatable will fail and cause problems. You use 'mkdir' without '-p' quite a few times, and these will exit with error codes in a second run.

A good way to test for idempotency is to deploy, immediately followd by 'juju debug-hooks vivo/0', and then you can just run the hook twice when the 'install' window opens in tmux/byobu. A good guideline to use is mkdir always gets '-p', mv, rm and cp always get '-f'.

* relation-get in config-changed will *always* fail. relation-get without any qualification is only valid in the *-relation-* hooks, unlesss you pass '-r' to set the relation-id. Instead you'll need to do something like this:

dbrel_id=`relation-ids|head -1`
if [ -z "$dbrel_id" ] ; then
  exit 0
fi
database=`relation-get -r $dbrel_id database`

You probably should also make it clear that you can only relate to one database service at a time, given that if you related to two, which one is chosen (above) is somewhat undefined.

Changed in charms:
assignee: Clint Byrum (clint-fewbar) → Nicholas Skaggs (nskaggs)
Nicholas Skaggs (nskaggs) wrote :

Can I get another review? How do I request one? :-)

nicholas, if you fixed the commented issues, you can change the bug status to "fix committed" or "new" and it'll be back in the queue

Nicholas Skaggs (nskaggs) wrote :

Nathan thank you. Somehow I missed your comment, so this sat for a bit. Ohh well, it's still ready to be looked at :-)

Changed in charms:
status: Incomplete → Fix Committed
Jorge Castro (jorge) on 2012-05-22
tags: removed: new-charm
Clint Byrum (clint-fewbar) wrote :

Hi Nicholas!

The changes look great. In the mean time since you did your changes, we added a new policy item. Charms must have a maintainer.

$ charm proof
E: Charms need a maintainer (See RFC2822) - Name <email>
W: missing recommended hook start
W: missing recommended hook stop

Also those warnings should be fixed. While start/stop don't currently get called all that often, in the near future charms will use these for things like live migration and backups. Please make sure you have a working start/stop hook.

Fix those three things from charm proof and this one is definitely ready for the store!

When you've fixed them, please re-subscribe 'charmers' to this bug and somebody will promulgate ASAP.

Changed in charms:
status: Fix Committed → Incomplete
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers