evertrue / zookeeper-cookbook

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

Fixed setting/interpolation of default[:zookeeper][:config_dir] attribute #161

Closed rmldsky closed 8 years ago

rmldsky commented 8 years ago

It seems that now default[:zookeeper][:config_dir] is set the way it ends up like this on node: config_dir: /opt/zookeeper/zookeeper-%{zookeeper_version}/conf. The zookeeper_version variable is never set in attributes/default.rb. Referencing node[:zookeeper][:config_dir] in wrapper cookbook ie. causes errors.

Also patch level version bump of cookbook.

jeffbyrnes commented 8 years ago

@rmldsky I should point out that variable is, in fact, set, please see line 5 of this very diff.

If you take a look at #153, you’ll see why this change was implemented, and how it operates.

Are you changing or, somehow, unsetting node[:zookeeper][:version] in your wrapper cookbook?

rmldsky commented 8 years ago

OK, spent some time checking out #153 and #151 . As far as I understand the reasoning and proposed solution, it still leaves me behind with not being able to reference node[:zookeeper][:config_dir] as it remains not interpolated (/opt/zookeeper/zookeeper-%{zookeeper_version}/conf).

This also leaves my PR being pointless/incomplete. I'll give a deeper look into this, if I am doing something wrong, or propose a broader solution (if any needed).

jeffbyrnes commented 8 years ago

I was going to expand on my original thoughts, but I see you’ve edited your comment to reflect a stronger understanding of how this work, so great!

If you want to use node[:zookeeper][:config_dir] elsewhere, e.g., in your wrapper cookbook, you’ll need to call it in the same fashion as #153 uses it:

"#{node[:zookeeper][:config_dir] % { zookeeper_version: node[:zookeeper][:version] }}/#{node[:zookeeper][:conf_file]}"

Might be best to do something like:

config_dir = "#{node[:zookeeper][:config_dir] % { zookeeper_version: node[:zookeeper][:version] }}"

if you’re planning on using it multiple times in a recipe of your own, since that gets pretty noisy.

I’m going to go ahead & close this out, since I don’t plan on reverting the behavior to what it was previously to #153. Please open a new PR if you feel you have an alternative solution!

neil-greenwood commented 8 years ago

Small point: the code included in the last comment for config_dir actually resolves to config_file inside the config directory. Nitpick, but might help if someone is cut-n-pasting the code...

jeffbyrnes commented 8 years ago

@neil-greenwood thanks for the catch! I’ve edited the comment to properly reflect the config_dir, w/o a trailing slash.