Merge lp://qastaging/~mbruzek/charms/precise/rabbitmq-server/tests into lp://qastaging/charms/rabbitmq-server

Reviewers: mp+202573_code.launchpad.net,

Message:
Please take a look.

Description:
Changed the REAME.md and pass charm proof

https://code.launchpad.net/~mbruzek/charms/precise/rabbitmq-server/tests/+merge/202573

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/51230045/

Affected files (+337, -21 lines):
   D README
   A README.md
   A [revision details]
   M config.yaml
   M hooks/_pythonpath.py
   M hooks/rabbit_utils.py
   M hooks/rabbitmq_server_relations.py
   M metadata.yaml
   A tests/00_setup.sh
   A tests/10_basic_deploy_test.py
   A tests/20_deploy_relations_test.py
   A tests/30_configuration_test.py
   A tests/rabbit-server-cert.pem
   A tests/rabbit-server-privkey.pem

Please take a look.

https://codereview.appspot.com/51230045/

Great start, see my comments below

https://codereview.appspot.com/51230045/diff/20001/tests/10_basic_deploy_test.py
File tests/10_basic_deploy_test.py (right):

https://codereview.appspot.com/51230045/diff/20001/tests/10_basic_deploy_test.py#newcode14
tests/10_basic_deploy_test.py:14: file_path = os.path.join(path,
'rabbit-server-privkey.pem')
You're sending SSL certs but never verify if they're actually working.
It'd be nice if this test also attempted an SSL connection to the
rabbitmq server and verify the certs were installed and setup properly

https://codereview.appspot.com/51230045/diff/20001/tests/20_deploy_relations_test.py
File tests/20_deploy_relations_test.py (right):

https://codereview.appspot.com/51230045/diff/20001/tests/20_deploy_relations_test.py#newcode39
tests/20_deploy_relations_test.py:39:
d.sentry.unit['rabbitmq-server/0'].relation('ceph', 'ceph:client')
While testing the ceph relation is important, you should also test the
amqp relation, as that's the primary purpose of this charm

https://codereview.appspot.com/51230045/diff/20001/tests/30_configuration_test.py
File tests/30_configuration_test.py (right):

https://codereview.appspot.com/51230045/diff/20001/tests/30_configuration_test.py#newcode54
tests/30_configuration_test.py:54: message = 'The certificate did not
match!'
Simply being able to check one of the several configuration options
isn't enough for a complete test. You should get the contents of the
configuration file (or if possible, query rabbitmq directly) to verify
the charm is doing the right thing

https://codereview.appspot.com/51230045/

https://codereview.appspot.com/51230045/diff/20001/config.yaml
File config.yaml (right):

https://codereview.appspot.com/51230045/diff/20001/config.yaml#newcode16
config.yaml:16: default: ""
I'd rather not see this - "" is not the same as unset and the charm uses
config-get --format=json ssl_key and relies on return values being
"None" in python.

https://codereview.appspot.com/51230045/

Please take a look.

https://codereview.appspot.com/51230045/

Please take a look.

https://codereview.appspot.com/51230045/

https://codereview.appspot.com/51230045/diff/40001/hooks/rabbit_utils.py
File hooks/rabbit_utils.py (right):

https://codereview.appspot.com/51230045/diff/40001/hooks/rabbit_utils.py#newcode1
hooks/rabbit_utils.py:1: #!/usr/bin/python
Why does rabbit_utils need to be executable? only the relations file
should be directly executed.

https://codereview.appspot.com/51230045/diff/60001/hooks/_pythonpath.py
File hooks/_pythonpath.py (right):

https://codereview.appspot.com/51230045/diff/60001/hooks/_pythonpath.py#newcode2
hooks/_pythonpath.py:2:
Again - why execute?

https://codereview.appspot.com/51230045/diff/60001/hooks/rabbit_utils.py
File hooks/rabbit_utils.py (right):

https://codereview.appspot.com/51230045/diff/60001/hooks/rabbit_utils.py#newcode2
hooks/rabbit_utils.py:2:
Why does this file need to be executable? typically the only file that
does is the hooks file.

https://codereview.appspot.com/51230045/

https://codereview.appspot.com/51230045/diff/60001/tests/20_deploy_relations_test.py
File tests/20_deploy_relations_test.py (right):

https://codereview.appspot.com/51230045/diff/60001/tests/20_deploy_relations_test.py#newcode58
tests/20_deploy_relations_test.py:58: d.configure('ceph',
ceph_configuration)
I don't think that the ceph charm is actually going to bring up a ceph
cluster with this configuration; by default it requires 3 service units
before it will actually startup; you can either add service units OR you
can set the mon-count config to 1 to fix this.

https://codereview.appspot.com/51230045/diff/60001/tests/20_deploy_relations_test.py#newcode113
tests/20_deploy_relations_test.py:113: print(key, value)
I think this needs more rigour; the ceph charm should have provided a
key for the rabbitmq charm to use; also where does print output go?

https://codereview.appspot.com/51230045/

Please re-assign charmers to this review when it's ready for re-review

Charm proof fails:
W: metadata name (rabbitmq-server) must match directory name (rabitmq-server) exactly for local deployment.
W: config.yaml: option ssl_key does not have the keys: default
W: config.yaml: option ssl_cert does not have the keys: default
W: config.yaml: option vip does not have the keys: default

Pulling the latest branch charm proof looks to be passing:

$ bzr pull
Using saved parent location: bzr+ssh://bazaar.launchpad.net/+branch/charms/rabbitmq-server/
No revisions or tags to pull.
$ charm proof
I: config.yaml: option key has no default value
I: config.yaml: option source has no default value
$

I chatted with Matt and he indicated additional work is needed in reponse to James Page's latest reviews. Thus marking this as "Needs Fixing."