Hi Liam,

Thank you for submitting the content-fetcher charm!  This is a really good idea for a functional charm and something I think would be very useful.  And I love that you have tests in this charm, that shows me that you took a lot of time to get this code done right!

I took a look at the charm and found a few things in my first review.


$ charm proof
W: config.yaml: option ssh_host_key does not have the keys: default
 
It looks like ssh_host_key does not have a default value.  I would suggest adding a default value of “” and make sure the code can properly handle empty strings.

The readme file properly converts from markdown to HTML and looks great!  You give some great examples of how to use content-fetcher with swift, bzr+ssh, file and lp.  That is great and will go a long way to give users information on how it works.

In “Testing the Charm” section there are two step number ones.  It looks like that could be because there are two separate actions to take depending on provider.  Maybe use 1) Testing the charm and use A) LXC and a separate section for other providers B) Other providers:

I noticed step 2) had an error on the last command.  The directory to the apache-vhost.txt is just “tests”.

I got an error when I tried to run step 3):  mbruzek@workhorse:/tmp/precise/content-fetcher$ ./tests/testserver/test-content-fetcher.py https
Traceback (most recent call last):
  File "./tests/testserver/test-content-fetcher.py", line 3, in <module>
    import netifaces
ImportError: No module named netifaces

The solution was straight forward, but something you want to call out in the readme so there are no errors.  Users would need to install python-netifaces before running your test server.

$ sudo apt-get install python-netifaces

If the Restrictions and Todo sections do not have any content I would remove those sections.

Looking at the code I found a few things that should be fixed.

The option “region_name” exists in config.yaml but I do not see it used anywhere in the code.  If the option is no longer needed it should be removed.

Updating the content within the charm seems like a good candidate for the upgrade-charm hook, but this hook does nothing.  Is there any reason why the code to install new content is not in the upgrade-charm hook?

The configuration options must be mutable.  While looking at the code in hooks.py it seems some of the configuration options (archive_location, tenant_name, auth_url, username, and keep_dir_count) are within an if block that will not be accessed on each config-changed event unless the deploy_trigger is greater than the current revision number.  This could create confusion for a user, who simply wants to change one of those values (say archive_location for example) but will not know why the charm has not updated.

I see you make reference to this unique behavior in the readme file as “Note that setting any other parameter will not trigger a deployment.”  I notice that the readme does not go into detail about how to increment this value to trigger a new deployment.  Juju users expect the charm to change when a configuration option is set and may not have read that section in the readme file.  Is it possible to rewrite the config-changed hook to read and change all the configuration options and only update the content in upgrade-charm hook?

Based on my review findings I am going to change the status of the bug to “incomplete”.  When you are ready for another review move the status to “new” or “fix committed”to get this back in the review queue.  

Thanks again for the work on this charm I look forward to the next revision.