Add a new api endpoint for /bundle/proof.

Add a new api endpoint for /bundle/proof.

Initial check is that we can resolve and locate the charms defined in the services block.

- Add a new BundleCharmDescription object which contains the important data about a charm and notes any assumptions made about how we'll attempt to identify that charm.
- Update the resolving code to use this new Description object vs the old manual methods
- Add the new api endpoint and attempt to load and resolve each of the services. If it cannot be resolved, an error message with a "process" list is added to the error messages to assist the user in understanding what the server did to attempt to locate the charm.

https://code.launchpad.net/~rharding/charmworld/api-proof/+merge/191037

(do not edit description out of merge proposal)

[rharding, 2013-10-14 20:07:40]

Please take a look.

[rharding, 2013-10-14 20:10:42]

Added reviewer notes

https://codereview.appspot.com/14441069/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/14441069/diff/1/Makefile#newcode15
Makefile:15: subversion nodejs
charm-tools is now a python dep and in download-cache.

https://codereview.appspot.com/14441069/diff/1/charmworld/testing/__init__.py
File charmworld/testing/__init__.py (right):

https://codereview.appspot.com/14441069/diff/1/charmworld/testing/__init__.py...
charmworld/testing/__init__.py:276: def load_data_file(filename):
needed to help load the sample data bundle.yaml in tests.

https://codereview.appspot.com/14441069/diff/1/charmworld/tests/test_models.py
File charmworld/tests/test_models.py (left):

https://codereview.appspot.com/14441069/diff/1/charmworld/tests/test_models.p...
charmworld/tests/test_models.py:1901: def
test_charm_resolving_from_bundle_branch(self):
moved to the new BundleCharmDescription tests.

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

https://codereview.appspot.com/14441069/diff/1/charmworld/views/api.py#newcod...
charmworld/views/api.py:507: if bundle_format == 'yaml':
starting out with only yaml support. Will do json in a follow up.

https://codereview.appspot.com/14441069/diff/1/charmworld/views/api.py#newcod...
charmworld/views/api.py:515: # Proof each bundle in the deployer file.
and this will get moved to a helper method when shared between yaml and json
files.

[bac, 2013-10-15 13:45:52]

Rick a very nice branch.  I've got some suggestions and one bug fix.  Should be
very straightforward and landable shortly.  Thanks for the thoughtful design.

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py
File charmworld/models.py (right):

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py#newcode1494
charmworld/models.py:1494: class BundleCharmDescription(object):
So this is a description of a charm in a bundle file, right?  I'm tempted to
suggest a tiny change to make it "BundledCharmDescription".  As it is currently,
I originally thought "BundleCharm" was 'bundle or charm', i.e. a generic
description that could describe either.

CharmDescriptionFromBundle?

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py#newcode1495
charmworld/models.py:1495: """Given the bundle yaml, describe a charm in the
services section"""
End with period.

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py#newcode1502
charmworld/models.py:1502: if branch.startswith('lp:'):
No need to do the test since the re.sub will have no effect if 'lp:' isn't the
start.

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py#newcode1522
charmworld/models.py:1522: """Given charm key info, build out Charm data to use
for later."""
A description of the other args would be useful.  Is charm a charm name? Calling
it charm_name might be clearer.

If neither charm nor charm_url are given, self.charm will be assigned None.  Is
that ok?

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py#newcode1524
charmworld/models.py:1524: charm = charm_url
This conflation of charm and charm_url is confusing to me.  Maybe have 'charm'
be the thing you've discovered from charm_name and charm_url.

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py#newcode1533
charmworld/models.py:1533: found_revision = re.search('-(\d)$', charm)
This regex doesn't work for digits > 9.  Needs to be

found_revision = re.search('-(\d+)$', charm)

And it'll need some test changes.  See:

http://paste.ubuntu.com/6240536/

(Yes I know the pastebin is ephemeral and I apologize.)

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py#newcode1575
charmworld/models.py:1575: charm = bundle_info.get('charm')
You never use charm again, so no need to assign it, especially since you don't
for charm_url.

https://codereview.appspot.com/14441069/diff/1/charmworld/testing/__init__.py
File charmworld/testing/__init__.py (right):

https://codereview.appspot.com/14441069/diff/1/charmworld/testing/__init__.py...
charmworld/testing/__init__.py:276: def load_data_file(filename):
Thanks for moving it here.  Long overdue.

https://codereview.appspot.com/14441069/diff/1/charmworld/tests/test_models.py
File charmworld/tests/test_models.py (right):

https://codereview.appspot.com/14441069/diff/1/charmworld/tests/test_models.p...
charmworld/tests/test_models.py:1941: service_data['branch'] =
'lp:charms/precise/mediawiki@7'
Make some of these two digits, as shown in my paste.  These don't exercise the
regex, though.

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

https://codereview.appspot.com/14441069/diff/1/charmworld/views/api.py#newcod...
charmworld/views/api.py:494: """Check the bundle provided for proof errors"""
.

https://codereview.appspot.com/14441069/diff/1/charmworld/views/api.py#newcod...
charmworld/views/api.py:495: # The top level if for general deployer file
errors.
"top level if" ?  I can't parse that.

https://codereview.appspot.com/14441069/diff/1/charmworld/views/api.py#newcod...
charmworld/views/api.py:507: if bundle_format == 'yaml':
On 2013/10/14 20:10:42, rharding wrote:
> starting out with only yaml support. Will do json in a follow up.

Really we have to support both?  Gross.

https://codereview.appspot.com/14441069/diff/1/charmworld/views/api.py#newcod...
charmworld/views/api.py:516: for key, value in bundle_data.items():
It would be a lot clearer if you used 'bundle_name' and 'bundle_data' instead of
generic key,value.

https://codereview.appspot.com/14441069/diff/1/charmworld/views/api.py#newcod...
charmworld/views/api.py:526: 'test', charm, value.get('series'))
Maybe instead of test you could use 'dummy_name' or something with 'proof'. 
Having 'test' in real code looks funny.

https://codereview.appspot.com/14441069/diff/1/charmworld/views/tests/test_ap...
File charmworld/views/tests/test_api.py (right):

https://codereview.appspot.com/14441069/diff/1/charmworld/views/tests/test_ap...
charmworld/views/tests/test_api.py:911: 
A test showing no errors would be gratifying.

[rharding, 2013-10-15 16:19:09]

Comments below. Pushing update branch shortly.

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py
File charmworld/models.py (right):

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py#newcode1494
charmworld/models.py:1494: class BundleCharmDescription(object):
On 2013/10/15 13:45:52, bac wrote:
> So this is a description of a charm in a bundle file, right?  I'm tempted to
> suggest a tiny change to make it "BundledCharmDescription".  As it is
currently,
> I originally thought "BundleCharm" was 'bundle or charm', i.e. a generic
> description that could describe either.
> 
> CharmDescriptionFromBundle?

I went with Bundled. I think it'll fit better when we get around to doing the
same thing for the bundle itself. To do relations, does it have charms, etc
there will need to be a BundleDescription. Basically a parsed file, but no real
model involvement.

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py#newcode1522
charmworld/models.py:1522: """Given charm key info, build out Charm data to use
for later."""
On 2013/10/15 13:45:52, bac wrote:
> A description of the other args would be useful.  Is charm a charm name?
Calling
> it charm_name might be clearer.
> 
> If neither charm nor charm_url are given, self.charm will be assigned None. 
Is
> that ok?

Ok cool, I know my :params: stuff was reading as line noise to benji before.
I've not found my balance on this stuff yet with everyone.

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py#newcode1524
charmworld/models.py:1524: charm = charm_url
On 2013/10/15 13:45:52, bac wrote:
> This conflation of charm and charm_url is confusing to me.  Maybe have 'charm'
> be the thing you've discovered from charm_name and charm_url.

Added a bunch of docs.

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py#newcode1533
charmworld/models.py:1533: found_revision = re.search('-(\d)$', charm)
On 2013/10/15 13:45:52, bac wrote:
> This regex doesn't work for digits > 9.  Needs to be
> 
> found_revision = re.search('-(\d+)$', charm)
> 
> And it'll need some test changes.  See:
> 
> http://paste.ubuntu.com/6240536/
> 
> (Yes I know the pastebin is ephemeral and I apologize.)
> 

<3 thanks so much for the catch and the pastebin. Phew.

https://codereview.appspot.com/14441069/diff/1/charmworld/models.py#newcode1575
charmworld/models.py:1575: charm = bundle_info.get('charm')
On 2013/10/15 13:45:52, bac wrote:
> You never use charm again, so no need to assign it, especially since you don't
> for charm_url.  

Thanks, updated.

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

https://codereview.appspot.com/14441069/diff/1/charmworld/views/api.py#newcod...
charmworld/views/api.py:495: # The top level if for general deployer file
errors.
On 2013/10/15 13:45:52, bac wrote:
> "top level if" ?  I can't parse that.

Heh, /if/is

https://codereview.appspot.com/14441069/diff/1/charmworld/views/api.py#newcod...
charmworld/views/api.py:516: for key, value in bundle_data.items():
On 2013/10/15 13:45:52, bac wrote:
> It would be a lot clearer if you used 'bundle_name' and 'bundle_data' instead
of
> generic key,value.

Bah, meant to fix that. I had issues where I was reusing a var name later in the
loop and meant to come up with better names here.

https://codereview.appspot.com/14441069/diff/1/charmworld/views/api.py#newcod...
charmworld/views/api.py:526: 'test', charm, value.get('series'))
On 2013/10/15 13:45:52, bac wrote:
> Maybe instead of test you could use 'dummy_name' or something with 'proof'. 
> Having 'test' in real code looks funny.

+1

https://codereview.appspot.com/14441069/diff/1/charmworld/views/tests/test_ap...
File charmworld/views/tests/test_api.py (right):

https://codereview.appspot.com/14441069/diff/1/charmworld/views/tests/test_ap...
charmworld/views/tests/test_api.py:911: 
On 2013/10/15 13:45:52, bac wrote:
> A test showing no errors would be gratifying.

Added

[rharding, 2013-10-15 16:23:22]

Please take a look.

[bac, 2013-10-15 17:35:16]

LGTM -- thanks for the nice changes.