Code review comment for lp:~jose/charms/precise/owncloud/port-change+repo+ssl-support

Revision history for this message
Matt Bruzek (mbruzek) wrote :

José,

Thank you for your submission for owncloud! Your continued dedication to the owncloud charm is very much appreciated.

I took some time to review the latest submission. The amulet test failed on hp-cloud with the error message:

HTTPSConnectionPool(host='15.125.126.129', port=443): Max retries exceeded with url: /index.php

We spoke about this and I understand this is because the 'domain' configuration value is not set after the deployment. We should try to set that and re-run the tests.

The tests/00_setup.sh tests need the executable flag.

Thank you for fixing the bugs I pointed out in the previous review. Also I really like the way you wrote the README.md with the Internet Connection Requirements section! Very well done sir, I think this will be a model for future charm README files.

config-changed:

I noticed something very strange in the config-changed hook:
sed -i 's/;upload_max_filesize/;upload_max_filesize/g' /etc/php5/apache2/php.ini

This seems to be searching for a term and replacing the exact same term. I believe this to be a mistake and it should be searching for '^upload_max_filesize” and replacing with the commented out version.

I realize this is not in your MP but please fix this problem.

Thanks for your time and work on this owncloud submission! I am going to put this MP to "needs fixing" and when you are ready for another review please click the "Request another review" button in the upper right hand corner of the commit message.

review: Needs Fixing

« Back to merge proposal