New collectd subordinate charm

[Jacek Nykis]

I wrote new collectd subordinate charm using the charm.reactive framework. I would like it to be a "recommended" charm so can I have please review for it?

The charm was tested using mojo but it currently does not have amulet tests. I tried writing them but I hit multiple problems doing that because my charm uses "juju-info" relation (and nobody in #juju was able to help).

Could I please get:
1. Review of the charm
2. Advice on how to write amulet tests

[Konstantinos Tsakalozos (kos.tsakalozos)]

Hi,

We would like to thank you for your time and effort you put into this charm.

It would be great if you could submit the repo with the layer you implemented in addition to the charm build output? This would make the review process much easier.
Guess it is this one https://code.launchpad.net/~jacekn/canonical-is-charms/collectd-composer , right?

In respect to your tests failing, we found the problem to be in line: http://bazaar.launchpad.net/~jacekn/charms/trusty/collectd/trunk/view/head:/tests/99-autogen#L14
You should a) be referencing explicitely the relation name for ubuntu and b) use the interface name of collectd.
So the line at hand should look like this: cls.deployment.relate('ubuntu:juju-info', 'collectd:host')

Can you also please make sure your "make test" is successful? You could use bundletester https://github.com/juju-solutions/bundletester to ensure your charm is ready.

Here are some additional comments we find important:

- We do not recomend using a hook to trap config changes.
  Many thanks to Cory Johns who tried to refactor your charm:
  https://code.launchpad.net/~johnsca/charms/trusty/collectd/layer-rq

- For subordinate charms we recommend you use the option:
  options:
    basic:
      use_venv: true
  In this way your subordinate charm will not conflict with the pricipal one as it will operate in a virtual environment.
  Have a look here: https://github.com/johnsca/layer-apache-hadoop-plugin/commit/861031a3a07a03ed6589065d1c59c40cbd1cdf9e

Many thanks to Cary John, and Kevin Monroe who helped in the review.

Thank you for your time,
Konstantinos

[Jacek Nykis (jacekn)]

Thank you for the review.

Cory's improvements worked fine, I merged them.

I added use_venv: true as suggested

And I fixed amulet tests, thank you for pointing out the fix.

New layer revision is in LP now:
https://code.launchpad.net/~jacekn/canonical-is-charms/collectd-composer

Can I have re-review?

[Konstantinos Tsakalozos (kos.tsakalozos)]

Thank you for taking the time to address the comments we had.

The tests run without any issue. We are missing a couple of minor fixes:
- The use_venv flag should go to the layers.yaml file not in metadata.yaml this was detected by charm proof.
- Style problems detected by make lint indicate we do not have any unit_test. We can silence that by providing a Makefile other than the one inherited by the basic layer. For other style problems inherited by the nrpe interface we contacted the author and they have been already fixed!

Please, consider merging the patch we offer on this branch:
https://code.launchpad.net/~kos.tsakalozos/collectd/almostthere

Thanks,
Konstantinos

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/2422/

[Jacek Nykis (jacekn)]

Hi Konstantinos,

Thank you for your branch, I merged it.

Could you or any other charmer have another look at the layer?

[Konstantinos Tsakalozos (kos.tsakalozos)]

Thank you for taking the time to address our comments. I am ok with moving on with this charm. Thank you.

[Kevin W Monroe (kwmonroe)]

I spotted an error in the write_graphite.conf.j2 template while testing this charm today. The 'prefix' config option should be named 'graphite_prefix'. While I was digging around, I came up with some suggestions for README changes. I've made the following MP to address both issues:

https://code.launchpad.net/~kwmonroe/canonical-is-charms/collectd-composer/+merge/288696

Please consider merging this into your layered source branch (at least the template fix), and we'll have another look. Thanks very much for your continued efforts in getting a collectd subordinate into the store!!

[Jacek Nykis (jacekn)]

Thank you Kevin for spotting the "graphite_prefix" problem.

Your branch looks good, I merged it.

[Tim Van Steenburgh (tvansteenburgh)]

Hey Jacek,

tvansteenburgh@xenial-vm:~/src/charms/layers/collectd-composer⟫ charm build
...
build: Please add a `repo` key to your layer.yaml, e.g. repo: http://bazaar.launchpad.net/~jacekn/canonical-is-charms/collectd-composer/
...

Can you please add this repo key to your layer.yaml? This will enable the `charm pull-source` to find the layer source for your charm.

[Jacek Nykis (jacekn)]

I don't get this error with stable charm-tools:
$ charm build
build: Composing into /home/jacek/charms/
build: Processing layer: layer:basic
build: Processing layer: collectd
build: Processing interface: nrpe-external-master

charm-tools:
  Installed: 1.11.2-0ubuntu1~ubuntu14.04.1~ppa1
  Candidate: 1.11.2-0ubuntu1~ubuntu14.04.1~ppa1
  Version table:
 *** 1.11.2-0ubuntu1~ubuntu14.04.1~ppa1 0
        500 http://ppa.launchpad.net/juju/stable/ubuntu/ trusty/main amd64 Packages

Also is this issue real blocker? It's been 2 months since I submitted the layer and so far nobody found critical bugs. Could we get it in the charmstore so that I can submit simple patches to resolve small problems?

[Marco Ceppi (marcoceppi)]

LGTM +1