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

Calculate download_url inside the recipe #179

Conversation

pivotal-casebook
Copy link

The attributes are being pre-calculated so they don't pickup
the custom version set by the user which can lead to issues
with the wrong version being installed.

If a user needs to create a custom download_url, they can
set the host and repository attributes in their wrapper cookbook
or on the chef server. For example to create the url:

http://example/downloads/elasticsearch-0.90.10.tar.gz

you set the attributes as:

default.elasticsearch[:version] = '0.90.10'
default.elasticsearch[:host] = 'http://example'
default.elasticsearch[:repository] = 'downloads'

Fixes #178

The attributes are being pre-calculated so they don't pickup
the custom version set by the user which can lead to issues
with the wrong version being installed.

If a user needs to create a custom download_url, they can
set the host and repository attributes in their wrapper cookbook
or on the chef server. For example to create the url:
http://example/downloads/elasticsearch-0.90.10.tar.gz
you set the attributes as:

default.elasticsearch[:version] = '0.90.10'
default.elasticsearch[:host] = 'http://example'
default.elasticsearch[:repository] = 'downloads'

Fixes sous-chefs#178
@martinisoft
Copy link

Just started running into this issue via a wrapper cookbook. 👍

@martinisoft
Copy link

Appears to also be a duplicate of #165

@martinisoft
Copy link

Never mind, I much prefer this approach to keep the download URL flexible.

@tyler-ball
Copy link

I'm actually a bigger fan of keeping the default.elasticsearch[:download_url] in the attributes file and getting rid of the default.elasticsearch[:host] and default.elasticsearch[:repository] attributes since they are not used anywhere except default.elasticsearch[:download_url].

Reasoning: It is pretty easy to overwrite an attribute in a wrapper cookbook. But if the cookbook constructs the URL with custom logic in a recipe, it is much harder to overwrite. What if I need to add user/password authentication to my URL login? It is harder to do that when the custom logic is embedded in a recipe.

Source: http://jtimberman.housepub.org/blog/2014/02/01/evolution-of-cookbook-development/ under the heading 'Composing URLs'

@karmi
Copy link
Contributor

karmi commented Mar 25, 2014

@tyler-ball Thanks for the reasoning, I agree with the line of thought.

If I remember correctly, the original idea was to allow installing from custom endpoints (eg. a custom fork of Elasticsearch, arbitrary URL within a company infrastructure, ...).

The only problem with having just default.elasticsearch[:download_url] would be that it's then quite confusing to use that attribute to set the version. And when we would extract the version from the download_url attribute as Joshua describes in the article for later use, we would probably end up with the same problems, only this time with default.elasticsearch[:version]...

@tyler-ball
Copy link

Yeah, I'm not sure if I agree with him about extracting the version from the download URL. With our wrapping of the cookbook we currently keep around both default.elasticsearch[:version] and default.elasticsearch[:download_url] (:download_url is constructed using :version). Sometimes, this requires us to reload attributes but that is fine with me.

The issue is in constructing attributes as composites of other attributes in cookbook A, then wrapping that with cookbook B. CHEF-4234 describes this well.

karmi pushed a commit that referenced this pull request May 7, 2014
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

Hi, I've merged #242, so this bug should be fixed now. 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
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quick Fix for version update issues
5 participants