Bug to track submission of content-fetcher charm to charm store

[Liam Young]

A subordinate charm which can be used to populate a directory with code from an external source. I think using this as a subordinate to the apache2 charm to populate the document root is the main use case at the moment.

The charms includes tests which I have run successfully with both the lxc and OpenStack providors

[Antonio Rosales (arosales)]

Liam,

Thanks for taking the time to submit content-fetcher for inclusion in the Juju charm store. Your contributions to improve the Juju Charm Store catalog are very appreciated.

This post is to let you know the ~charmers teams has seen this submission and will be giving this charm an in-depth review shortly.

If you have any questions please feel free to ping us in #juju on Freenode.

-thanks,
Antonio

[Matt Bruzek (mbruzek)]

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 ...

[Liam Young (gnuoy)]

Hi Matt,

thank you for a thorough review I really appreciate it. I think I've addressed all the points you raised.

1) ssh_host_key now has a default. Although I'm not a fan of this charm proof test tbh Bug #1277652
2) I've simplified the README so that information about tests is restricted to instructions on invoking the makefile test target. This included removing the instructions on how to run the tests with the test server running locally for lxc tests which removes the netifaces problem.
3) region_name has been removed from config.yaml
4) Downloading content and installing is a potentially expensive operation so I was keen to avoid doing it on every config change and I also lean on the side of preserving upgrade-charm for actions relating to upgrading the charm code. However, on reflection I think you are right and this causes the charm to behave in a unintuitive way. So, I've introduced the update_on_change option which causes the charm to update code on every config-change and upgrade-charm hook execution. Switching update_on_change to false causes the charm to require a deploy-trigger increment to trigger a deployment.

Any other issues, recommendations or gripes please don't hesitate to bounce it back.
Thanks again,
Liam

[Liam Young (gnuoy)]

I forgot to say if testing on trusty you'll need lp:~gnuoy/charms/precise/apache2/24-auth-format unless it's been merged

[Charles Butler (lazypower)]

just looking through the repository - it appears the above patch has been merged in... continuing with the review.

[Charles Butler (lazypower)]

Liam,

I apologize for the delay between my initial contact and the follow up.

I've deployed and reviewed the charm code for the content-fetcher and things appear to be in order with the charm. I've run several tests on the provided fetching providers (except swift) and it worked really well! I don't see anything particularly heinous in the charm however the included tests are failing to run using the readme instructions.

What I'm seeing when they are run, under each fetching provider - a traceback is being thrown from juju. (attached below)

I'm going to bounce this back to you for a brief look at the traceback to see if this was User Error or if something has changed and the tests need to be refactored. Don't hesitate to ping me directly once you've had a chance to address the issue below for a re-review.

Thanks for the continued effort Liam!

export JUJU_REPOSITORY=../../
juju bootstrap
make runtests

...

ERROR service "content-fetcher" not found
ERROR service "content-fetcher" not found
panic: runtime error: index out of range

goroutine 1 [running]:
runtime.panic(0xcc1300, 0x19ec9b7)
        /usr/lib/go/src/pkg/runtime/panic.c:266 +0xb6
main.(*SetCommand).Run(0xc2100fa400, 0xc2100df3c0, 0x0, 0x0)
        /build/buildd/juju-core-1.20.1/src/github.com/juju/juju/cmd/juju/set.go:92 +0x62d
github.com/juju/juju/cmd/envcmd.(*environCommandWrapper).Run(0xc2100f9ec0, 0xc2100df3c0, 0xb27740, 0xc21010a430)
        /build/buildd/juju-core-1.20.1/src/github.com/juju/juju/cmd/envcmd/environmentcommand.go:1 +0x3d
main.(*envCmdWrapper).Run(0xc2100f9ee0, 0xc2100df3c0, 0x0, 0x0)
        /build/buildd/juju-core-1.20.1/src/github.com/juju/juju/cmd/juju/addmachine.go:1 +0x3d
github.com/juju/cmd.(*SuperCommand).Run(0xc210171160, 0xc2100df3c0, 0xc2100df3c0, 0x0)
        /build/buildd/juju-core-1.20.1/src/github.com/juju/cmd/supercommand.go:321 +0x3ca
github.com/juju/cmd.Main(0x2b7a195de3e8, 0xc210171160, 0xc2100df3c0, 0xc21000a010, 0x3, ...)
        /build/buildd/juju-core-1.20.1/src/github.com/juju/cmd/cmd.go:247 +0x283
main.Main(0xc21000a000, 0x4, 0x4)
        /build/buildd/juju-core-1.20.1/src/github.com/juju/juju/cmd/juju/main.go:77 +0xada
main.main()
        /build/buildd/juju-core-1.20.1/src/github.com/juju/juju/cmd/juju/main.go:174 +0x44

goroutine 3 [syscall]:
os/signal.loop()
        /usr/lib/go/src/pkg/os/signal/signal_unix.go:21 +0x1e
created by os/signal.init·1
        /usr/lib/go/src/pkg/os/signal/signal_unix.go:27 +0x31

goroutine 12 [syscall]:
runtime.goexit()
        /usr/lib/go/src/pkg/runtime/proc.c:1394
^Cmake: *** [runtests] Interrupt