Juju Charms Collection
apache2 package

Code review comment for lp:~evarlast/charms/trusty/apache2/add-logs-interface

  1. Trusty Tahr (14.04)
  2. add-logs-interface
  3. Merge into trunk
Revision history for this message
Cory Johns (johnsca) wrote :

Jay,

Thanks for addressing those test failures. I think your solution is reasonable, and the tests now pass for me. I think that this mostly looks good, except for two small items.

First, I think there's a corner case in how you write out to the logs relation, that I mentioned in my diff comment below. I think just dropping the call to relation_ids() entirely should resolve it without issue.

Second, I would like to see some documentation in the README about how this is supposed to be used. You mention logstash in the MR, above, but that charm doesn't support this relation directly. I'm guessing that you intend this to be used via the beaver charm (e.g., https://jujucharms.com/u/yellow/beaver/trusty/0) because that has an interface that seems to match up, but it's not at all clear.

With those two small changes, I would give this my +1.

« Back to merge proposal