Finished charm using services framework

Finished charm using services framework



https://code.launchpad.net/~johnsca/charms/trusty/cf-hm9000/services/+merge/221770

(do not edit description out of merge proposal)

[johnsca, 2014-06-02 17:49:21]

Please take a look.

[benjamin.saller, 2014-06-02 18:29:44]

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.co...
templates/cf-hm9k-analyzer.conf:2: author "Cory Johns
<cory.johns@canonical.com>"
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

[johnsca, 2014-06-02 19:43:47]

On 2014/06/02 18:29:44, benjamin.saller wrote:
> 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.

+1 I definitely should have done this to begin with.

>
https://codereview.appspot.com/104820043/diff/1/templates/cf-hm9k-analyzer.co...
> templates/cf-hm9k-analyzer.conf:2: author "Cory Johns
> <mailto:cory.johns@canonical.com>"
> 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.

Easy enough to do in this charm during this review, then we can fix the existing
ones in a batch.

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

This raises an issue with how I implemented multiple units in the framework,
which I will address now.

[johnsca, 2014-06-02 22:14:06]

Please take a look.

[benjamin.saller, 2014-06-04 21:02:23]

Thanks for this, since it depends on the helpers change lets resolve that before
I can approve this. Good stuff though.

https://codereview.appspot.com/104820043/diff/20001/metadata.yaml
File metadata.yaml (right):

https://codereview.appspot.com/104820043/diff/20001/metadata.yaml#newcode3
metadata.yaml:3: maintainer: cf-charmers
ha, nice catch. we might want to use the full email address for cf-charmers
though, or say lp:~cf-charmers. Something to make it a little more clear.

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

https://codereview.appspot.com/104820043/diff/20001/templates/hm9000.json#new...
templates/hm9000.json:14: {% for unit in etcd.values() -%}
This might change a little if you like the helpers changes suggested in the
other branch. Thanks for this important fix.

[johnsca, 2014-06-05 18:17:46]

Please take a look.

[benjamin.saller, 2014-06-06 16:45:31]

This LGTM,+1

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

https://codereview.appspot.com/104820043/diff/40001/templates/hm9000.json#new...
templates/hm9000.json:14: {% for unit in etcd -%}
nice, thanks