Code review comment for lp:~jaypipes/glance/bug713126

Revision history for this message
Rick Harris (rconradharris) wrote :

Excellent work Jay. I got it working with my CloudFiles account with some small-fixes:

> 237 + account = options.get('swift_store_account',
> 238 + DEFAULT_SWIFT_ACCOUNT)

Account is referenced but not used. AFAIK, there isn't a distinction between
account and username, so, I think we can remove all references to account.

> 143 -from __future__ import absolute_import

Was this removed for a reason? The `swift` adapter is shadowing the `swift`
module so `import swift.common.client` won't do the right thing without this.

> 215 + def add(cls, id, data, options, conn_class=None):

We probably shouldn't be using `id`, but, we can save that for a cleanup-patch
where we fix that everywhere, all-at-once.

> 308 + "location %(location)s" % locals())

One space to the right :)

> 334 + except Exception, e:

This should probably be `except swift.common.client.ClientException, e` in
order to guarantee that it has the `http_status` attribute.

Since, you'll need swift.common.client in the local-namespace to do this, you
can either:

* Use separate imports in each function that has to use something from the
  swift module

* Use a lazy-import pattern: define a global SWIFT global variable (initialized to None) and then lazily populate it. Then use that to access sub-modules. I believe this is done in Nova.

> 281 + obj_etag = swift_conn.put_object(container, id, data)

Needs to be `str(id)` since the `quote` method used by swift.Client expects a
string.

Since this is used repeatedly (in the head_object call too), probably best to
do something like:

    obj_name = str(id)

at the top of each method.

Currently we're assuming that the container already exists (if it doesn't, the
PUT will fail with a 404). I would vote that we automatically create the
container if it doesn't exist.

Perhaps this behavior should be governed by a configuration option, something
like `swift_store_create_container_on_put`

review: Needs Fixing

« Back to merge proposal