Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(323)

Issue 14789043: Add charm config proofing and update api response

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 6 months ago by rharding
Modified:
7 years, 6 months ago
Reviewers:
mp+191689, gary.poster
Visibility:
Public.

Description

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)

Patch Set 1 #

Patch Set 2 : Add charm config proofing and update api response #

Total comments: 23
Unified diffs Side-by-side diffs Delta from patch set Stats (+413 lines, -43 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
A charmworld/lib/proof.py View 1 1 chunk +76 lines, -0 lines 7 comments Download
A charmworld/lib/tests/__init__.py View 1 0 chunks +-1 lines, --1 lines 0 comments Download
A charmworld/lib/tests/test_proof.py View 1 1 chunk +151 lines, -0 lines 2 comments Download
M charmworld/views/api.py View 1 3 chunks +87 lines, -39 lines 12 comments Download
M charmworld/views/tests/test_api.py View 1 4 chunks +98 lines, -5 lines 2 comments Download

Messages

Total messages: 3
rharding
*** Submitted: Add charm config proofing and update api response - Move proof logic into ...
7 years, 6 months ago (2013-10-18 19:09:23 UTC) #1
rharding
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#newcode501 charmworld/views/api.py:501: 'errors': [], this is the list ...
7 years, 6 months ago (2013-10-18 19:36:14 UTC) #2
gary.poster
7 years, 6 months ago (2013-10-18 20:10:28 UTC) #3
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b