evertrue / zookeeper-cookbook

Chef cookbook for installing and managing Zookeeper.
https://supermarket.chef.io/cookbooks/zookeeper
Apache License 2.0
81 stars 119 forks source link

overriding Zookeeper version attribute does not affect resulting mirror URL #77

Closed timoreimann closed 10 years ago

timoreimann commented 10 years ago

Zookeeper's mirror URL is directly derived from the version attribute:

default[:zookeeper][:version] = "3.4.5"
default[:zookeeper][:mirror] = "http://mirrors.ibiblio.org/apache/zookeeper/zookeeper-#{default[:zookeeper][:version]}/zookeeper-#{default[:zookeeper][:version]}.tar.gz"

It seems that overriding the version field does not change the mirror URL subsequently because the latter will already have been computed with the default version value ("3.4.5" as of now). I had to override the mirror attribute in my wrapper cookbook, which implies repeating the base URL and missing out on a potential future URL change in the Zookeeper cookbook.

It would be great if all that was required to pull in a different Zookeeper version was specifying the desired version attribute. The Opscode Cookbook Style Guide suggests to _not calculate or derive values in attributes files, [but] use a recipe instead_. I believe the Zookeeper recipe already does something similar when building the version-dependent installation path (e.g., /opt/zookeeper/zoopeeker-3.4.5), so hopefully a similar approach should work for the mirror URL too.

I am by no means a Chef expert, so let me know if I missed something. If not, please let me know if you would like to me to submit a pull request. Would be more than happy to help out on this one.

jakedavis commented 10 years ago

Hey @timoreimann, I'm a big fan of "attributes which derive their values from other attributes should be in recipes" and it's a convention we follow internally. I'll take care of this today.

timoreimann commented 10 years ago

Hey @jakedavis, thanks for the prompt response. Glad to hear you'll take care of things.

jakedavis commented 10 years ago

Nice catch, thank you. This was fixed in #78.