Comment 6 for bug 964936

Brandon, I finally got some time to take a look at this. I am reviewing the one on launchpad,

lp:~imbrandon/charms/oneiric/drupal/trunk/

At revno 11.

If your github version has progressed and they are different, please sync to launchpad so we're on the same page. Overall it looks *great*. I really like the way most of the smarts is left up to nginx/php and this just puts files in place.

==== Fixes needed ====

* install does not set -e
 - Almost all of these commands may fail. Install needs to detect and report errors. set -e will solve that.

* Once you do that, install is not idempotent. While it does not run multiple times, it may have an error and need to be retried:
 - line 9, it uses 'mv'. This will fail on a retry of install. See below where 'install' is used.
 - everywhere "ln -s" is used should be replaced with 'ln -sf' so symlinks are created/overwritten unconditionally.

* Remove all mention of s3cmd, its not used.

* Do the git clone of pressflow in install, not db-relation-changed. There is no need to keep doing that over and over again if the database is unrelated/related again. Think of the scenario where you migrate from a small to a large DB instance. Also you need to check for its existence before doing it, otherwise the hook will fail because it already exists.

* in db-relation-changed:

if [ -f "$config_file_path" ]; then
  juju-log "Drupal is already setup, just silently going away"
  exit 0
fi

Since you don't remove this file on departed/broken, this will cause the config file to not be updated if the relation is removed/added again. I see no reason to have this check, just re-write the config file with the new values every time.

==== Fixes that can wait ====

* start does not detect errors in php5-fpm (it will accidentally detect an error in nginx, because bash reports the exit code of its last command).
* stop also does not properly detect errors
* private_name=`hostname -f` -- line 7 of db-relation-changed, dead code, remove it.

==== Next steps ====

Marking as Incomplete. When your fixes are done, mark it as Fix Committed and I'll review agian.

Thanks!