Skip to content
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

Conversation

organicveggie
Copy link

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.

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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Contributor

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants