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

Move download_url resolving to recipe #227

Closed
wants to merge 1 commit into from

Conversation

quatrix
Copy link

@quatrix quatrix commented Jul 22, 2014

This makes changing elasticsearch version easier from wrapper cookbooks

This makes changing elasticsearch version easier from wrapper cookbooks
@karmi
Copy link
Contributor

karmi commented Jul 22, 2014

Hi, thanks!, although, this has been debated across couple of issues, and the downside of a patch like this is that you cannot then set a custom URL in your attributes.

As I said across multiple issues, the cookbook needs to be refactored, to prevent this problems, have a better test infrastructure etc...

I don't have time right now to evaluate the patch, this is something we should consider for the rewrite.

@quatrix
Copy link
Author

quatrix commented Jul 23, 2014

good point!

what if it does:

download_url = node.elasticsearch[:download_url].empty? ? [node.elasticsearch[:host], node.elasticsearch[:repository], filename].join('/') : node.elasticsearch[:download_url]

will you accept or just prefer to refactor the whole thing?

thanks

@karmi
Copy link
Contributor

karmi commented Sep 3, 2014

For reference, there's an interesting approach suggested in https://coderanger.net/derived-attributes/, where Noah Kantrowitz proposes interpolating the dynamic parts of attributes with %{}, which sounds promising.

karmi pushed a commit that referenced this pull request Oct 13, 2014
This prevents a number of errors and surprises when setting the Elasticsearch version.

If the user overrode the node.elasticsearch[:version] property, the download directory and symlinks would be
created with the correctly overridden version number. However, the download URL would be computed with
the default version number, and thus the incorrect, default version of elasticsearch would be installed.

Since there's no need to compute the download filename and URL until node convergence, this patch moves that computation to the default.rb recipe. However, the node.elasticsearch[:filename] and node.elasticsearch[:download_url] attributes, if present, are respected and given preference upon node convergence, so that users still have the capability to override these attributes and thus configure downloads from a custom repository.

Related: #100, #180, #178, #240, #179, #245, #227, #245

Closes #242
@karmi
Copy link
Contributor

karmi commented Oct 13, 2014

@quatrix, thanks for the patch!! I've merged #242 now, so this bug should be fixed. I'm sorry for the inconvenience the bad behaviour caused you. Closing this issue for now, please ping me if you would like to discuss it further.

@karmi karmi closed this Oct 13, 2014
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