#401

[dmp42]

fix #401 + bump the version to 0.7.1

If this is ok:

• will cherry pick this into master

We may wait a bit to release 0.7.1, or release it right away (what do you think?).

[dmp42]

Also see #403 for open questions on release management / branching strategy.

[shin-]

Can you add this to the 0.7.1 changelog? 3faf029

otherwise lgtm!

[wking]

On Mon, Jun 02, 2014 at 02:20:34AM -0700, Mangled Deutz wrote:

• Fix "storage_path" configuration is not reflected. #401

Instead of extracting cfg.get('storage_path') and passing it
separately to storage class, can't we just pass the config instance
and leave path-extraction (where appropriate) up to the class itself?
I can certainly imagine storage class implementations that don't need
a base path.

[dmp42]

@wking strong coupling between an opaque config object (registry side) and driver is IMO bad, and I want to break/reduce it.

I would really prefer explicit arguments whenever possible. Indeed, most if not all drivers have a concept of base path (or prefix), which is why this one should really be different from the other driver-specific configs.

Just thinking aloud, though - and admittedly, that point wasn't discussed when the abstract interface was designed, but it can sure be now.

Best.

[dmp42]

@shin- sure! I'll look into it tomorrow morning - ping me if you want anything else into 0.7.1 before I pypi it.

[shin-]

It's already in master, just having it documented in the Changelog.md file would be great. :)

[dmp42]

@shin- it's on master but not on "stable/0.7" (which I cut from 0.7.0).
Don't bother though - I'll backport it.
Please focus on docker 1.0 for now and keep it rolling! We will discuss all that later.

Best.

[wking]

Mangled Deutz wrote:

strong coupling between an opaque config object (registry side) and driver is IMO bad, and I want to break/reduce it.

Agreed. It would be nice to have a namespace for all storage-side config options, and then something like:

kwargs = {k[len('config_'):]: cfg.get(k, None) for k in cfg.keys() if k.startswith('config_')}
engine.fetch(kind)(**kwargs)

With a ConfigParser-based config file format, the kwarg extraction would just be:

kwargs = config['storage']

in Python 3.2 and:

kwargs = {k: config.get('storage', k) for k in config.options('storage')}

in older Pythons.

The specifics of a driver's config are completely orthogonal to the rest of the registry core, so anything other than opaque config forwarding seems overcomplicated.

Indeed, most if not all drivers have a concept of base path (or prefix), which is why this one should really be different from the other driver-specific configs.

Maybe all existing drivers do, but I could imagine a storage backend based on a database that doesn't need a base path or prefix. In any case, this is still guessing about the semantics of a hypothetical (or at least external) driver's config, and I don't see any gains by doing that in the core code.

[dmp42]

Maybe all existing drivers do, but I could imagine a storage backend based on a database that doesn't need a base path or prefix.

We can imagine it - then it would be an exception to the general rule, and it should just ignore the argument (as the path arg is optional anyhow).

Either way, even with a database, a "translated" path make sense (table prefixes for eg).

In any case, this is still guessing about the semantics of a hypothetical (or at least external) driver's config, and I don't see any gains by doing that in the core code.

The gain is that whenever / if we change the configuration format for storage_path, we don't need to patch ALL drivers, but just the registry (mapping between the config key and the arg) - which is exactly what happens here - I don't want to touch the driver itself.

Anyhow, the future may be like what we did for Hipache - use uris as a configuration provider for most "common" variables (see /~https://github.com/dotcloud/hipache/tree/master/lib/drivers).

eg:

• s3://bucketname/storage_path
• file://storage_path
• protocol://login:password@host:port/database_or_path_or_whatever#prefix

The benefit is that uris are a well-defined way to represent said information, and are both expressive and compact notations.

Still need to review the drivers and see if that make sense - but this is getting OT - more on that later!

Thanks for the review!

[wking]

On Tue, Jun 03, 2014 at 02:14:20AM -0700, Mangled Deutz wrote:

In any case, this is still guessing about the semantics of a
hypothetical (or at least external) driver's config, and I don't
see any gains by doing that in the core code.

The gain is that whenever / if we change the configuration format
for storage_path, we don't need to patch ALL drivers, but just the
registry (mapping between the config key and the arg) - which is
exactly what happens here - I don't want to touch the driver itself.

Why have a mapping between the config key and the arg that's more than
“strip off a prefix that only the registry side knows about, leaving
the tail that only the storage side knows about”. I agree that it
would be a terrible idea to translate a STORAGE_PATH config to a
‘prefix’ argument.

Anyhow, the future may be like what we did for Hipache - use uris
as a configuration provider for most "common" variables (see
/~https://github.com/dotcloud/hipache/tree/master/lib/drivers).

I'm fine with that too, since it's just another way to make the
storage config completely opaque to the core code.

Still need to review the drivers and see if that make sense - but
this is getting OT - more on that later!

Fair enough :p. And sorry for drilling down on these design decisions
after they land. You're too fast for me to notice them while they're
in flight ;).