fgrehm / vagrant-cachier

Caffeine reducer
http://fgrehm.viewdocs.io/vagrant-cachier
MIT License
1.08k stars 111 forks source link

multiple generic buckets #99

Closed arosenhagen closed 10 years ago

arosenhagen commented 10 years ago

according to the docs this is the way to have multiple generic buckets:

Vagrant.configure("2") do |config|
    config.cache.enable :generic, { :name => "one", :cache_dir => "/var/cache/one" }
    config.cache.enable :generic, { :name => "two", :cache_dir => "/var/cache/two" }
end

correct me if I'm wrong, but this way we only get generic-two as it overrides the first one. It should be more like this (?):

Vagrant.configure("2") do |config|
    config.cache.enable :generic, [
        { :name => "one", :cache_dir => "/var/cache/one" }
        { :name => "two", :cache_dir => "/var/cache/two" }
    ]
end
fgrehm commented 10 years ago

Oh, this is a documentation and implementation bug as the current codebase is not capable of handling multiple generic buckets :-( I'll try to have a look at it when I have a chance.

/cc @gnustavo

gnustavo commented 10 years ago

I see. Config.enable uses the bucket name as a key in the buckets hash, so we can't call it more than once for the same bucket type.

In order to make multiple generic buckets work we have to change the way we pass their configuration to Config.enable. @arosenhagen proposal to use an array of hashes is nice because it allows us to keep the "syntax" to configure a single bucket the same. The Bucket.Generic.install method would simply have to check if it gets an array or a hash as the configs argument and act accordingly.

However, what do you think about this alternative syntax?

Vagrant.configure("2") do |config|
  config.cache.enable :generic, {
    "wget" => { :cache_dir => "/var/cache/wget" },
    "curl" => { :cache_dir => "/var/cache/curl" },
  }
end

This way, the configs argument is always a hash mapping a "generic bucket name" to its specific configuration hash. It allows for a unique "syntax" for one or multiple generic buckets. It also avoids the "redundant" :name key and makes it clear that names should not clash.

The downside of this is that we would have to make a backwards incompatible change to the plugin configuration syntax. Unless we still allow for single nameless bucket being configured with its direct config hash, like this:

Vagrant.configure("2") do |config|
  config.cache.enable :generic, { :cache_dir => "/var/cache/wget" }
end

We could detect it by the presence of the :cache_dir key in the config hash.

Of course, the simplest solution would be to remove any mention of multiple generic buckets from the documentation. :-)

I vote for the hash-of-hashes syntax with an allowance for the single nameless generic bucket to keep it backwards compatible.

I'd like to know what do you think before proposing a pull request.

fgrehm commented 10 years ago

I vote for the hash-of-hashes syntax with an allowance for the single nameless generic bucket to keep it backwards compatible.

Let's do this!

gnustavo commented 10 years ago

Hi @fgrehm, have you noticed my pull request (#101) trying to solve this?

fgrehm commented 10 years ago

This is fixed and will be available with 0.7.1!