Code review comment for lp:~johnsca/charms/trusty/cf-hm9000/services

Revision history for this message
Benjamin Saller (bcsaller) wrote :

Thanks for this, a little hard to verify as you indicated. It looks like
we can add the internal MCAT suite to the CI as well.

I think landing this now and completing the topology is fine, even if we
have to evolve its configuration its an 'optional' component.

LGTM

https://codereview.appspot.com/104820043/diff/1/hooks/config.py
File hooks/config.py (right):

https://codereview.appspot.com/104820043/diff/1/hooks/config.py#newcode24
hooks/config.py:24: 'required_data': [contexts.NatsRelation(),
What do you think about a top level define of this list that we reuse,
something like:

required_data: hm_contexts

where hm_contexts is defined above the service block.

https://codereview.appspot.com/104820043/diff/1/templates/cf-hm9k-analyzer.conf
File templates/cf-hm9k-analyzer.conf (right):

https://codereview.appspot.com/104820043/diff/1/templates/cf-hm9k-analyzer.conf#newcode2
templates/cf-hm9k-analyzer.conf:2: author "Cory Johns
<email address hidden>"
This is an existing issue, but I'd like for all the Authors in all the
projects to point to the projects list rather than a bunch of
individuals. The charms maintainer fields as well. We should make a card
for this and switch them at once.

https://codereview.appspot.com/104820043/diff/1/templates/hm9000.json
File templates/hm9000.json (right):

https://codereview.appspot.com/104820043/diff/1/templates/hm9000.json#newcode13
templates/hm9000.json:13: "store_urls":
["http://{{etcd['hostname']}}:{{etcd['port']}}"],
This could easily be a list, no? I think we'll want to handle it that
way (via iteration), but for now this is fine.

https://codereview.appspot.com/104820043/diff/1/templates/hm9000.json#newcode15
templates/hm9000.json:15: "metrics_server_port": 7879,
Making two cards, been meaning to audit all the default ports and
username/password defaults. This is something we'll need to be able to
manage better in the future. Nothing to do in this branch

https://codereview.appspot.com/104820043/

« Back to merge proposal