Add charm config proofing and update api response

Add charm config proofing and update api response

- Move proof logic into a new lib/proof.py module
- Update the proof api call to look through the bundles and proof them
- Change errors to be catchable exceptions, the exception contains both a msg
and debug_info we expose via the api call
- Update the api call to provide a summary error_messages (for Marco's needs)
but keep the details in the actual list of errors with the messages and the
debug info we have that went into those errors.

Sample proof api call and response:
http://paste.mitechie.com/show/1049/


Note: this is only adding the new proof checks of validating the
config/options from the charm found in the database to the options defined in
the bundle.

- Validate the option exists in the charm we found in the db
- Validate that the type is of the correct type.
- Validate that the charm we found has options at all.

QA
---
You have to ingest the charms so that we can validate them. You can then toss
the yaml file (as a single string) to the proof api endpoint as a POST'd key
deployer_file. Right now it only handles yaml.

https://code.launchpad.net/~rharding/charmworld/proof-config/+merge/191689

(do not edit description out of merge proposal)

[rharding, 2013-10-18 19:09:23]

*** Submitted:

Add charm config proofing and update api response

- Move proof logic into a new lib/proof.py module
- Update the proof api call to look through the bundles and proof them
- Change errors to be catchable exceptions, the exception contains both a msg
and debug_info we expose via the api call
- Update the api call to provide a summary error_messages (for Marco's needs)
but keep the details in the actual list of errors with the messages and the
debug info we have that went into those errors.

Sample proof api call and response:
http://paste.mitechie.com/show/1049/


Note: this is only adding the new proof checks of validating the
config/options from the charm found in the database to the options defined in
the bundle.

- Validate the option exists in the charm we found in the db
- Validate that the type is of the correct type.
- Validate that the charm we found has options at all.

QA
---
You have to ingest the charms so that we can validate them. You can then toss
the yaml file (as a single string) to the proof api endpoint as a POST'd key
deployer_file. Right now it only handles yaml.

R=
CC=
https://codereview.appspot.com/14789043

https://codereview.appspot.com/14789043/diff/3001/charmworld/lib/proof.py
File charmworld/lib/proof.py (right):

https://codereview.appspot.com/14789043/diff/3001/charmworld/lib/proof.py#new...
charmworld/lib/proof.py:4: from charmworld.models import (
I'm not a fan of importing models into a lib file, open to ideas of ways to make
this better other than passing in a resolver and a descriptor into the proof
calls from the view.

https://codereview.appspot.com/14789043/diff/3001/charmworld/lib/proof.py#new...
charmworld/lib/proof.py:54: if not charm.options:
this is only called if it's expecting to proof options. So not having any is an
error. (test_key and test_value are required inputs)

https://codereview.appspot.com/14789043/diff/3001/charmworld/lib/proof.py#new...
charmworld/lib/proof.py:67: # # The config key is a valid one for this charm.
Check that the value
doh, will clean up.

[rharding, 2013-10-18 19:36:14]

Reviewer comments added.

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py
File charmworld/views/api.py (right):

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py#new...
charmworld/views/api.py:501: 'errors': [],
this is the list of actual errors. They can be global messages ('could not parse
the file') or objects from error checking a specific bundle in the file.

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py#new...
charmworld/views/api.py:502: 'error_messages': [],
this is a summary of all error messages provided as an aid to marco.

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py#new...
charmworld/views/api.py:503: 'debug_info': [],
and a global level 'debug_info' to match the representation of errors of other
areas of the code where they all provide a message as well as helpful debug
information to assist in finding the way to correct the error.

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py#new...
charmworld/views/api.py:514: try:
moved the old checks to the try:catch for the ProofError. I felt this read a bit
cleaner and allows for a function to return a value (the parsed yaml file)
normally, but have a complex object come back during a failure.

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/tests/test...
File charmworld/views/tests/test_api.py (right):

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/tests/test...
charmworld/views/tests/test_api.py:905:
results['errors'][0]['services']['db'][0]['message'])
this is due to the move to support multiple errors on the same service within a
bundle.

[gary.poster, 2013-10-18 20:10:28]

Hi Rick.  Thank you for a nice branch.  LGTM.

I give a ton of comments that could lead to a decent amount of work.  Please
treat all of them as suggestions, though I do think that the code in the
BundleProof code ought to move back to the view, somehow, and I hope I made at
least a somewhat compelling case for it.

https://codereview.appspot.com/14789043/diff/3001/charmworld/lib/proof.py
File charmworld/lib/proof.py (right):

https://codereview.appspot.com/14789043/diff/3001/charmworld/lib/proof.py#new...
charmworld/lib/proof.py:4: from charmworld.models import (
On 2013/10/18 19:09:24, rharding wrote:
> I'm not a fan of importing models into a lib file, open to ideas of ways to
make
> this better other than passing in a resolver and a descriptor into the proof
> calls from the view. 

Similar alternative: You have to instantiate BundleProof with these extra bits,
to create something that the view can use?  Don't know charmworld, so I don't
know where it would be stashed, but that broad approach seems tried and true.

https://codereview.appspot.com/14789043/diff/3001/charmworld/lib/proof.py#new...
charmworld/lib/proof.py:20: def check_parseable_deployer_file(data,
format='yaml'):
Ah, ye olde naming problem: a validation function also does work.  :-)  If you
really cared about the naming problem, I would suggest naming this
"parse_deployer_file".  The fact that you convert the error (while tossing out
any helpful information from the parser's error, btw :-/) seems minor compared
to the fact that this parses the data.  <shrug> (which means "do as you will")

https://codereview.appspot.com/14789043/diff/3001/charmworld/lib/proof.py#new...
charmworld/lib/proof.py:41: return charm_found
Uh oh, this validation function is doing work too. :-/

Um.

This doesn't feel right.  The Bundle proof bits here feel like they belong in an
integration layer, like a view, rather than in a library.  Do with that what you
will.

https://codereview.appspot.com/14789043/diff/3001/charmworld/lib/proof.py#new...
charmworld/lib/proof.py:54: if not charm.options:
On 2013/10/18 19:09:24, rharding wrote:
> this is only called if it's expecting to proof options. So not having any is
an
> error. (test_key and test_value are required inputs)

Is this an error of the bundle author or in the charmworld code?  It feels like
the latter, in which case raising a ProofError seems wrong.  We ought to log
something instead.  If I'm wrong, nevermind.

https://codereview.appspot.com/14789043/diff/3001/charmworld/lib/tests/test_p...
File charmworld/lib/tests/test_proof.py (right):

https://codereview.appspot.com/14789043/diff/3001/charmworld/lib/tests/test_p...
charmworld/lib/tests/test_proof.py:19: sample_deployer_file = """
...This is JSON...

https://codereview.appspot.com/14789043/diff/3001/charmworld/lib/tests/test_p...
charmworld/lib/tests/test_proof.py:33: """A deployer file should be parseable as
yaml."""
"...as YAML or JSON."?

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py
File charmworld/views/api.py (right):

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py#new...
charmworld/views/api.py:497: def _proof_bundle(self, request):
I think this method is getting too big.  Could we divide it up into logical
chunks, so one can read what's going on in the top level, and then dig down as
necessary?

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py#new...
charmworld/views/api.py:501: 'errors': [],
On 2013/10/18 19:36:14, rharding wrote:
> this is the list of actual errors. They can be global messages ('could not
parse
> the file') or objects from error checking a specific bundle in the file.

Add as comment?

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py#new...
charmworld/views/api.py:502: 'error_messages': [],
On 2013/10/18 19:36:14, rharding wrote:
> this is a summary of all error messages provided as an aid to marco.

s/marco/proof/ and add as comment?

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py#new...
charmworld/views/api.py:503: 'debug_info': [],
On 2013/10/18 19:36:14, rharding wrote:
> and a global level 'debug_info' to match the representation of errors of other
> areas of the code where they all provide a message as well as helpful debug
> information to assist in finding the way to correct the error.
Add as comment? :-)

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py#new...
charmworld/views/api.py:514: try:
On 2013/10/18 19:36:14, rharding wrote:
> moved the old checks to the try:catch for the ProofError. I felt this read a
bit
> cleaner and allows for a function to return a value (the parsed yaml file)
> normally, but have a complex object come back during a failure.

Ah, I see.  I essentially was suggesting the old approach.

Because of the concerns you and I both raised, I personally lean towards
believing that keeping it here in the view code--albeit perhaps in a separate
function, for readability, as you said--is a better approach.

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py#new...
charmworld/views/api.py:534: # Verify we can find the charms at all.
Per my suggestion to divide this up a bit, could this be one method?

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py#new...
charmworld/views/api.py:537: charm_found = BundleProof.check_service_exists(
As before: in this case I think keeping the code here in the view module--in a
separate function, perhaps--might be a better choice.

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/api.py#new...
charmworld/views/api.py:550: # config is valid for the values allowed by the
charm we found.
Per my suggestion to divide this up a bit, could this be another method?

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/tests/test...
File charmworld/views/tests/test_api.py (right):

https://codereview.appspot.com/14789043/diff/3001/charmworld/views/tests/test...
charmworld/views/tests/test_api.py:983: def
test_bundle_proof_supports_multiple_errors(self):
If you divide up the big validation method, you could test smaller chunks too. 
I like these tests, though.