Code review comment for lp:~jose/charms/precise/owncloud/update-version

Charles Butler (lazypower) wrote :

Hello again Jose,

Prior to merging I wrote this review while I was still actively reviewing a few fringe cases I had not thought of. I did not upgrade from a 4.x to 6.x release, which is an important factor. Presently this will break owncloud for users that have existing deployments.

The upgrade-charm hook has some syntax errors on the find command, -exec blocks need to be terminated with an escaped semicolon like so:

 # Excise old installation data
 find /var/www -mindepth 2 -maxdepth 2 -type d -name 'data'\
  -exec mv {} /var/tmp/ \;

and it should be idempotent. If the upgrade hook fails, and is re run in its current state it will wipe out the backed up data. Since you didn't write this hook I spent a few cycles working over the hook and have some proposed changes, but its not 100%, as the deployment will still break the application in unknown ways.

Could you spend some time looking at upgrading the upgrade-charm hooks as well? This is an area that we need to pay extra special attention to as data integrity of existing deployments must be upheld. Thanks in advance for any cycles you can put into this. If it proves to be too difficult, let me know and i'll build off of the work you have here and attempt the upgrade.

I'm going to mark the review as needs work. When you are ready for another review be sure to click the request another review button and assign charmers. Someone will be along shortly to review your work.

If you have any questions, comments, concerns join us in #juju on and/or email the mailing list at <email address hidden>

Thanks again for all the diligent work!

review: Needs Fixing

« Back to merge proposal