C4-89 Always use S3BlobStorage

[willronchetti]

• At some point registry.settings[BLOBS] became set and we stopped using S3BlobStorage. We never use Postgres for blobs so remove code that does this from register_storage

[netsettler]

Because this matters a lot, I'm going to make my comment a blocker. I'll look at the fix in place before approving.

[netsettler]

I worked through this by cases and believe this is almost right, but I don't like the use of try to avoid doing a simple conditional, plus it doesn't do the right thing in the case of a null string in registry.settings['blob_bucket'], which won't give a KeyError nor even an error at all, it will end up just making a S3BlobStorage(""), which I'd rather not leave to chance because it might err immediately or it might err later.

blob_bucket = registry.settings['blob_bucket']
if blob_bucket:
    registry[BLOBS] = S3BlobStorage(blob_bucket)
else:
    registry[BLOBS] = RDBBlobStorage(registry[DBSESSION])

or even the following, which could be done on one line, but isn't as easy to read. The main reason to do this version would be to emphasize the fact that the assignment is unconditional and only the value to be assigned is conditional.

blob_bucket = registry.settings['blob_bucket']
registry[BLOBS] = (S3BlobStorage(blob_bucket)
                   if blob_bucket 
                   else RDBBlobStorage(registry[DBSESSION]))

By the way, one thing I puzzled about in reviewing this code is why we previously tested the value of something we were going to unconditionally assign, so that's why unconditional assignment is on my mind. I'm glad that test went away, as it didn't seem to be helping.

There is, by the way, an odd case I noticed working through this where the previous value is a blob storage of the other kind and you just suddenly change because the blob_bucket has been set or unset. I think that case can never happen unless you make a mechanism to dynamically assign blob_bucket (which we might do), and in that case, I think it's proper to go ahead with such a change of type. I just wanted to note here that I thought about that case and am OK with the basic flow proposed here even if we later make such a change.

[willronchetti]

Will accept this, but shouldn't registry.settings['blob_bucket'] use .get()?

[netsettler]

Yep. Good catch. Please consider that fix part of my proposed rewrite.

[netsettler]

This looks good. Thanks for integrating your fix to mine. :)