Exodus to remove comma-sep constraints.

Exodus to remove comma-sep constraints.

Comma-separated constraints in bundles are deprecated.  Currently newly
processed bundles are converted to space-separated on ingestion but there are
a number of old bundles in the db that need fixing.

As this is an exodus, and only one can be present at a time, the 025 exodus is
deleted.

https://code.launchpad.net/~bac/charmworld/comma-migration-2/+merge/216585

(do not edit description out of merge proposal)

[bac, 2014-04-21 13:19:45]

Please take a look.

[j.c.sackett, 2014-04-21 14:09:36]

Brad--

This looks good, thanks. I just have a few comments/questions in the diff.

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

https://codereview.appspot.com/89770044/diff/1/charmworld/models.py#newcode1904
charmworld/models.py:1904: return original_bundle_data != bundle_data
Is there any value to this return? We don't seem to use it anywhere.

I could see a value for this as a test, but we have no test for
convert_bundle_constraints.

It might be worth it to add that test, but the function is tested in both the
migration and (I assume) the ingest code.

If it's not tested in ingest, we need a test for this function.

https://codereview.appspot.com/89770044/diff/1/charmworld/models.py#newcode1916
charmworld/models.py:1916: convert_bundle_constraints(bundle_data)
Given you don't care about the keys in `data` wouldn't it be cleaner to just do 

for bundle_data in data.values():

https://codereview.appspot.com/89770044/diff/1/charmworld/models.py#newcode1928
charmworld/models.py:1928: 
I don't object, but I'm curious what you gain by moving this into its own
function--I don't see this being reused anywhere. Did I miss it? Is it just
neater here?

Also, is save_file implicitly tested anywhere? If not, esp with the extra logic
for assigning content_type, it probably should have its own test.

[bac, 2014-04-21 14:44:23]

Please take a look.

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

https://codereview.appspot.com/89770044/diff/1/charmworld/models.py#newcode1904
charmworld/models.py:1904: return original_bundle_data != bundle_data
On 2014/04/21 14:09:36, j.c.sackett wrote:
> Is there any value to this return? We don't seem to use it anywhere.

There is not.  Originally I was using it but then removed it when I realized the
value needed to be stored in the target whether it was changed or not.

> 
> I could see a value for this as a test, but we have no test for
> convert_bundle_constraints.
> 
> It might be worth it to add that test, but the function is tested in both the
> migration and (I assume) the ingest code.

The functionality was tested implicitly but I'll add an explicit unit test.

> 
> If it's not tested in ingest, we need a test for this function.

https://codereview.appspot.com/89770044/diff/1/charmworld/models.py#newcode1916
charmworld/models.py:1916: convert_bundle_constraints(bundle_data)
Good idea.

https://codereview.appspot.com/89770044/diff/1/charmworld/models.py#newcode1928
charmworld/models.py:1928: 
On 2014/04/21 14:09:36, j.c.sackett wrote:
> I don't object, but I'm curious what you gain by moving this into its own
> function--I don't see this being reused anywhere. Did I miss it? Is it just
> neater here?

It was part of a refactoring that I turned out to not need but I thought it was
cleaner.  Upon further review, if no one else needs I've decided to revert the
change.  YAGNI and all.

[j.c.sackett, 2014-04-21 14:55:42]

LGTM. Thanks, Brad.