New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix custom installation directory #80

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@organicveggie

organicveggie commented Mar 28, 2013

If you set a custom installation directory using the :dir attribute, the cookbook still installs ElasticSearch in /usr/local/. This is because the call to ark that downloads and installs ElasticSearch doesn't include the necessary parameters to actually place the unpacked files in a custom location.

This pull request updates the default cookbook to properly configure ark when using a custom directory.

Show outdated Hide outdated recipes/default.rb
ark_prefix_root = node.elasticsearch[:dir]
ark_prefix_root ||= node.ark[:prefix_root]
ark_prefix_home = node.elasticsearch[:dir]
ark_prefix_home ||= node.ark[:prefix_home]

This comment has been minimized.

@karmi

karmi Apr 6, 2013

Member

@organicveggie Thanks for this, I would bet custom locations have worked in the past, maybe before Ark integration or some Ark chage. However, why set the attributes here, in the recipe file? Why not just use node.elasticsearch[:dir]? To allow them setting node.ark[:prefix_root] globally? Shouldn't we then do something like node.ark[:prefix_root] || node.elasticsearch[:dir] ?

@karmi

karmi Apr 6, 2013

Member

@organicveggie Thanks for this, I would bet custom locations have worked in the past, maybe before Ark integration or some Ark chage. However, why set the attributes here, in the recipe file? Why not just use node.elasticsearch[:dir]? To allow them setting node.ark[:prefix_root] globally? Shouldn't we then do something like node.ark[:prefix_root] || node.elasticsearch[:dir] ?

@karmi

This comment has been minimized.

Show comment
Hide comment
@karmi

karmi May 9, 2013

Member

@organicveggie Can we revive this discussion? First, setting node attributes in recipes (and not attributes) is tricky with latest Chef versions, second, isn't there an easier way with Ark? @bryanwb, can you maybe chime in, please?

Member

karmi commented May 9, 2013

@organicveggie Can we revive this discussion? First, setting node attributes in recipes (and not attributes) is tricky with latest Chef versions, second, isn't there an easier way with Ark? @bryanwb, can you maybe chime in, please?

@organicveggie

This comment has been minimized.

Show comment
Hide comment
@organicveggie

organicveggie May 9, 2013

@karmi Sorry about the incredible delay in responding. I apparently missed your original comment on this pull request - somehow it slipped through the cracks of my email. :(

Couple thoughts. First, you wrote:

setting node attributes in recipes (and not attributes) is tricky with latest Chef versions

Can you explain what you mean by that? I'm not quite sure I understand.

Second, you asked if there is an easier way with Ark. I'd like to think so and I'm definitely not an expert on leveraging Ark. But... this appeared to be the only way to make it work. It's quite possible I missed something in Ark though.

Third, you're right on why the pull-request uses both node.elasticsearch[:dir] and node.ark[:prefix_root]. If the cookbook user doesn't set a directory for elasticsearch but defined prefix directories for ark, it seemed reasonable that we should respect that. More to the point, the user can customize some of the other behaviors of Ark by setting attributes and it would feel weird if some worked and others (like prefix_root) didn't work.

However, it seems to me that if the user defines an elasticsearch directory, that should be preferred over the global ark directory. So maybe a tidier way of expressing that would be along the lines you suggested:

ark_prefix_root = node.elasticsearch[:dir] || node.ark[:prefix_root]
ark_prefix_home = node.elasticsearch[:dir] || node.ark[:prefix_home]

organicveggie commented May 9, 2013

@karmi Sorry about the incredible delay in responding. I apparently missed your original comment on this pull request - somehow it slipped through the cracks of my email. :(

Couple thoughts. First, you wrote:

setting node attributes in recipes (and not attributes) is tricky with latest Chef versions

Can you explain what you mean by that? I'm not quite sure I understand.

Second, you asked if there is an easier way with Ark. I'd like to think so and I'm definitely not an expert on leveraging Ark. But... this appeared to be the only way to make it work. It's quite possible I missed something in Ark though.

Third, you're right on why the pull-request uses both node.elasticsearch[:dir] and node.ark[:prefix_root]. If the cookbook user doesn't set a directory for elasticsearch but defined prefix directories for ark, it seemed reasonable that we should respect that. More to the point, the user can customize some of the other behaviors of Ark by setting attributes and it would feel weird if some worked and others (like prefix_root) didn't work.

However, it seems to me that if the user defines an elasticsearch directory, that should be preferred over the global ark directory. So maybe a tidier way of expressing that would be along the lines you suggested:

ark_prefix_root = node.elasticsearch[:dir] || node.ark[:prefix_root]
ark_prefix_home = node.elasticsearch[:dir] || node.ark[:prefix_home]
@karmi

This comment has been minimized.

Show comment
Hide comment
@karmi

karmi May 11, 2013

Member

@organicveggie Thanks for the update!

setting node attributes in recipes (and not attributes) is tricky with latest Chef versions
Can you explain what you mean by that? I'm not quite sure I understand.

Doing node.elasticsearch[:dir] = 'foo' is not allowed (see http://docs.opscode.com/breaking_changes_chef_11.html), and generally there's lots of confusion about how attribute precedence/overloading work.

I agree 100% that we should honor any ark setting provided by the user -- I'll try to investigate this a bit.

@bryanwb, can you help us here a bit?

As a side note, I think we should have elasticsearch::tarball with the current behaviour, and elasticsearch::package which would install from deb/rpm packages <-- what do you thiknk, @vhyza?

Member

karmi commented May 11, 2013

@organicveggie Thanks for the update!

setting node attributes in recipes (and not attributes) is tricky with latest Chef versions
Can you explain what you mean by that? I'm not quite sure I understand.

Doing node.elasticsearch[:dir] = 'foo' is not allowed (see http://docs.opscode.com/breaking_changes_chef_11.html), and generally there's lots of confusion about how attribute precedence/overloading work.

I agree 100% that we should honor any ark setting provided by the user -- I'll try to investigate this a bit.

@bryanwb, can you help us here a bit?

As a side note, I think we should have elasticsearch::tarball with the current behaviour, and elasticsearch::package which would install from deb/rpm packages <-- what do you thiknk, @vhyza?

@organicveggie

This comment has been minimized.

Show comment
Hide comment
@organicveggie

organicveggie May 14, 2013

@karmi I hear what you're saying, but we're not actually setting a node attribute... ark_prefix_root and ark_prefix_home are just local variables that exist only during the Chef run.

organicveggie commented May 14, 2013

@karmi I hear what you're saying, but we're not actually setting a node attribute... ark_prefix_root and ark_prefix_home are just local variables that exist only during the Chef run.

@karmi

This comment has been minimized.

Show comment
Hide comment
@karmi

karmi May 14, 2013

Member

@organicveggie I see, sorry for the confusion. I promise I'll try to test & verify this soon. Sorry I'm reluctant to pull it in "just like that", usually I end up supporting any feature and need to understand it in order to do so effectively :)

Member

karmi commented May 14, 2013

@organicveggie I see, sorry for the confusion. I promise I'll try to test & verify this soon. Sorry I'm reluctant to pull it in "just like that", usually I end up supporting any feature and need to understand it in order to do so effectively :)

@organicveggie

This comment has been minimized.

Show comment
Hide comment
@organicveggie

organicveggie May 14, 2013

@karmi Oh, no worries at all! I''m glad you're cautious rather than just accepting changes.

One thing, I should make one more change and follow your advice to tidy up the lines for ark_prefix_root and ark_prefix_home. Let me do that and update the pull request.

organicveggie commented May 14, 2013

@karmi Oh, no worries at all! I''m glad you're cautious rather than just accepting changes.

One thing, I should make one more change and follow your advice to tidy up the lines for ark_prefix_root and ark_prefix_home. Let me do that and update the pull request.

@karmi

This comment has been minimized.

Show comment
Hide comment
@karmi

karmi May 14, 2013

Member

@organicveggie Yes, please, that'd be great.

Member

karmi commented May 14, 2013

@organicveggie Yes, please, that'd be great.

@karmi karmi closed this in cb22871 Jun 10, 2013

karmi added a commit that referenced this pull request Jun 10, 2013

@karmi

This comment has been minimized.

Show comment
Hide comment
@karmi

karmi Jun 10, 2013

Member

Sean, sorry for the ridiculous delay... Merged the changes, verified in Vagrant, fixed related issue with plugin directory. Thanks!

Member

karmi commented Jun 10, 2013

Sean, sorry for the ridiculous delay... Merged the changes, verified in Vagrant, fixed related issue with plugin directory. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment