New charm: chamilo

[José Antonio Rey]

Here is the submission of a Chamilo charm.

Chamilo is an e-learning and content management platform.

Tests are on the way for it, but not on the initial charm. Still need to make my way through them ;)

[Matt Bruzek (mbruzek)]

Greetings José,

Thank you for submitting the Chamilo charm! What an interesting application it will be a great addition to the Juju charm store.

You are well known to have charm proof with no warnings or errors. Once again you have achieved victory over the charm proof tool! Great job.

The README is in Markdown and looks great when it is converted. The content is clear and I was able to use it to deploy Chamilo successfully.

hooks/install
The code in here is clean and easy to follow. As you know it is charm store policy to verify the software that you download, your juju-log statement claims to verify the download but I do not see the sha1sum verification of the downloaded tar file. You will need to fix this before this charm is accepted in the charm store.

Also I see that you change the permissions to 777 on some of the files in /var/www. I think a better and more secure way is to change the owner of these files to www-data and www-data as the group. This can be done by running the following command: chown -R www-data:www-data /var/www/

hooks/config-changed
The README indicates the version is locked at 1.9.6. The version configuration option is never read in the config-changed file. If the option is not used please remove it from config.yaml.

If the configuration changes either by a config-changed event or a db-relation-changed event, would a restart of the server be in order? Looking at this code I do not see how the apache server can be restarted once it is started. See my comment about hooks/stop.

hooks/stop
This hooks look good, but stop never removes the .started file. This may be a problem if you need to restart the service.

hooks/db-relation-changed
I removed the database relation and chamilo had a user friendly error message: “Database is unavailable”. That is excellent! However when I added the relation back in I was unable to use chamilo after that. Something in the logic must need to tell chamilo that the database is back.

Over all I thought this was a very good charm, the code was easy to read and it was well done. Thank you so much for your submission!

Based on the cryptographic verification being policy I have moved this bug to Incomplete. Once you've addressed the issues above either with code changes or comments, and wish to have another review, simply move the bug back to either "New" or "Fix Committed" to have it added back to the review queue.

If you have any questions or comments about the review contact mbruzek in IRC. You can find the rest of the team in #juju on irc.freenode.net or email the mailing list <email address hidden>

[Yannick Warnier (ywarnier)]

Hi Matt, I'm the development leader for Chamilo. I have no experience with charms but helped José find his way through Chamilo. Here are my observations... (hoping José can pick up his good work soon)

I added chamilo-1.9.6.1.tar.gz.sha1 on the server (we used md5 in the charm).

Permissions can, indeed, be changed from 777 to www-data. No issue for Chamilo there.

As far as I understand José's intention, version 1.9.6.1 is hardcoded because, at this time, there is no other supported version (meaning we didn't test any other version). In terms of Chamilo, the only two use cases of the version number is during the PHP installation process (which we elegantly skipped here) and to add it to the Chamilo configuration file in main/inc/conf/configuration.php.

The only configuration that would require a reload (but a restart is never required) of Apache would be the change of hostname (if this hostname is defined as a virtual host after Apache's start on the machine). That's all.

Regarding the database connection issue... I'm not sure the issue is with Chamilo. Each time you load a page, Chamilo will check if the database is there before sending the signal. However, if you removed and restored the instance, would it be possible that it might have changed IP or something? In this case, the configuration file of Chamilo (php format) should be updated with the new settings.

[Matt Bruzek (mbruzek)]

Hello Yannick,

Your interest in this charm is great, thank you for responding to my review.

I am a charmer with no experience with Chamilo. As the Subject Matter Expert you know what it needs and how it works better than I ever could. I just wanted to respond to your comments in order here.

Thank you for adding the sha1 file on your site! As I mentioned to José he could have computed the SHA1 sum on his computer, put the value in the code and checked the sha1 sum against that value. It is charm store policy to cryptographically verify downloads before installing them. We suggest SHA1 because a flaw has been found in MD5 (http://en.wikipedia.org/wiki/MD5 third paragraph).

A hard coded version of Chamilo is fine. I was simply pointing out there was a configuration parameter which would presumably allow users to change the version but the code never reads the version configuration parameter.

Currently there are no reloads in the Chamilo charm code, and apache2 only gets started once. If the database parameters change to point to a different database (via the db-relation-changed hook) I suspected that a reload is in order, but did not see where that would be called.

When I disconnected the database and reconnected the same database the IP, user/pass had not changed so I suspected there was something wrong in the charm code and it requires more investigation (perhaps a reload). I am sure José will try that case and figure out what needs to change.

Thanks again for your interest in this charm. Feel free to leave another comment or contact me in #juju on Freenode my handle is mbruzek.

[Yannick Warnier (ywarnier)]

Hi Matt,

Being a charmer sure sounds nice.

I had a quick chat with José on IRC (#chamilo) and he told me he would look into it as soon as he was done with two other charms he's building. I'll be around to help him when he needs it.
It's a matter of curiosity for me to see how users will use the charm and how it might affect Chamilo's number of installed portal (currently around 13,500 and growing 600 more each month).

[José Antonio Rey (jose)]

Hey, Matt and Yannick!

The charm code should be fixed now, so it's ready for review. If you find any issues feel free to report and I'll make sure to address them as soon as possible.

[Yannick Warnier (ywarnier)]

Hi Matt,

We're releasing Chamilo 1.9.8 (which has no structure or install process changes) tomorrow, so having an idea of whether the charm is OK for you now would help prepare the announcements for next week (whether we mention something about the charm or not) :-)

[Matt Bruzek (mbruzek)]

Jose,

Thank you for working on the chamilo charm!

I took another look at the code today and found the config-changed hook failed after I performed the following steps:

juju deploy --repository=../../ local:precise/chamilo
juju deploy mysql
juju add-relation chamilo mysql
juju set chamilo domain=10.0.3.53 user='admin' pass='juju'
juju status
...
agent-state-info: 'hook failed: "config-changed"'

Because of this deployment error I am going to set this bug to Incomplete. Set it back to new when you have addressed the problem and we will take another look.

Thanks again for the work here Jose!

[Cory Johns (johnsca)]

José,

This charm looks great, but for one issue. Charm proof runs without errors or warnings, the README is complete and informative, the config options are all handled properly, including when not set, and the charm gracefully handles losing the database, including having a new one added with different configuration.

However, when attempting to change the pass config option after having set it previously, the charm went into an error state on config-changed, and the following message was in the logs:

2014-06-25 23:33:28 INFO config-changed /var/lib/juju/agents/unit-chamilo-0/charm/hooks/config-changed: line 45: juju-log Updating password: command not found
2014-06-25 23:33:28 ERROR juju.worker.uniter uniter.go:486 hook failed: exit status 127

[José Antonio Rey (jose)]

Hey Cory,

This is due to a typo which fix has already been pushed to the branch. Thanks!

[Cory Johns (johnsca)]

José,

Great! With that fix, everything seems perfect. Have my +1! :)

[Marco Ceppi (marcoceppi)]

Hi Jose,

This charm passes my review. There are several knitpicks which I will highlight below.

It's be great to have the version of chamilo available as configuration

There's no tests (either unit or functional)

Otherwise this is promulgated! Thanks!