Code review comment for lp:~evarlast/charms/trusty/kibana/add-port-path-config

Revision history for this message
Charles Butler (lazypower) wrote :

Greetings Jay,

Thanks for this contribution. I've taken some time to look this over, and I have a few comments:

There's nothing that stands out about the code - however it would be nice to have the tests updated to flex the configuration option and validate that the resulting service is available on the port/path specified with the new configuration options. This will prevent us from breaking it in the future.

I did however notice that the charm when deployed via the tests results in failure. You can view the results here: http://reports.vapour.ws/charm-test-details/charm-bundle-test-parent-229

The Install hook is the culprit for the deployment failure. DEBUG:runner: unit: kibana/0: machine: 1 agent-state: error details: hook failed: "install" So I cannot block on this - but I will block on the lack of test coverage of the new options.

I'm happy to work with you on updating the tests if you need assistance to cover the new options. Feel free to reach out via IRC.

Otherwise, this MP looks good and aside from the minor feedback above is nearly ready for inclusion in the charm.

Thanks again for the submission. I'm going to change status of this MP to "needs work" and when you're ready switch merge status to 'needs review' and someone will be along shortly to review your work.

If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>, or ask a question tagged with "juju" on http://askubuntu.com.

review: Needs Fixing

« Back to merge proposal