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

Allow any custom configuration #186

Closed

Conversation

kamaradclimber
Copy link

When defining custom analyzer you need to specify full yaml instead of pure key values.
This patch is not perfect but does the job.

@karmi
Copy link
Contributor

karmi commented Feb 13, 2014

Hmmm, so you want Chef to place a file with settings on the node? Why not create the index with correct settings or use an index template ?

@kamaradclimber
Copy link
Author

I want chef to allow any configuration style allowed by elasticsearch (not getting in the way).
thanks for the pointer I also have a look

@kamaradclimber
Copy link
Author

index templates are great (thanks!) but does not reduce the need for being able to set any kind of settings in the elasticsearch.yml

@karmi
Copy link
Contributor

karmi commented Feb 15, 2014

@kamaradclimber
Copy link
Author

Yes I know this feature but it forces you to specify the all hierarchy at each line instead of using readable yaml

foo.bar.hello: world
foo.bar.filter.0: lowercase
foo.bar.filter.1: stop

instead of

foo:
  bar:
    hello: world
    filter:
    - lowercase
    - stop

The print_value method is a nice helper to fetch information from several sources but, at least for now, it constrains to inline configuration

@kamaradclimber
Copy link
Author

Hi @karmi ,
is there anything I can improve in this PR ?

@karmi
Copy link
Contributor

karmi commented Mar 10, 2014

Hi Grégoire, I'm sorry, but I'm really against pulling this in... I'm feeling this piles another way how to configure stuff with the cookbook, a way which needs to be tested, properly explained and documented, supported in issues...

I can understand this might be hard, but if the use case is configuring index analysis, I'm 100% against it -- this should be handled on index creation or with index templates, as I suggest above.

@kamaradclimber
Copy link
Author

No feelings hurt, I completely understand the need to limit the supported features.

I followed your advice and use index templates for my use case.

However the need for this feature is real, I use it to configure http.compression, cluster.routing.allocation.cluster_concurrent_rebalance parameters.

I see several ways to allow to configure it without this PR:

  • allow custom attributes to be added with print_value (for instance an attribute [:elasticsearch][:custom_conf] as an array of string containing key to be printed by print_value)

in attributes:

default[:elasticsearch][:custom_conf] = [
  'http.compression',
  'cluster.routing.allocation.cluster_concurrent_rebalance'
]

in the template elasticsearch.yml.erb:

<% node['elasticsearch']['custom_conf'].each do |k| %>
<%= print_value k -%>
<% end %>

that would allow anyone to add attributes (in a role or application cookbook).
This would remove the nice indentation but it is not really that bad.

  • allow to specify the cookbook parameter of the elasticsearch.yml resource:
template "elasticsearch.yml" do
  path   "#{node.elasticsearch[:path][:conf]}/elasticsearch.yml"
  source "elasticsearch.yml.erb"
  cookbook node.elasticsearch['configuration_cookbook']
  owner node.elasticsearch[:user] and group node.elasticsearch[:user] and mode 0755
  notifies :restart, 'service[elasticsearch]' unless node.elasticsearch[:skip_restart]
end

with default[:elasticsearch']['configuraton_cookbook'] = 'elasticsearch' in attributes

  • add an api lwrp (I have defined a small one for interacting with index templates). This is far more complex but would be a complete solution.

What do you think?

@kamaradclimber
Copy link
Author

btw the second option resembles to #153 (but the suggestion is to reopen the resource which raises a warning during chef-client…)

@karmi
Copy link
Contributor

karmi commented Mar 10, 2014

Yeah, all valid options, let me add my opinions:

add an api LWRP

This is the best option -- but the most demanding one :), as it would basically mean refactoring the whole cookbook. That may actually happen some time, but right now, I can't dedicate any significant effort into it.

allow to specify the cookbook parameter of the elasticsearch.yml resource

I believe this is what #153 is about -- I'm all for this, given that the behaviour is properly tested and documented. I can look into it over time if nobody picks it up...

allow custom attributes to be added with print_value

This feels like twisting Chef really too much -- and the cookbook does a lot of twisting already :) Let's focus on adding the cookbook parameter way, though I'm fairly certain that the "wrapper cookbook" is both safer and more maintainable approach.

@kamaradclimber
Copy link
Author

as an start, here is what we use to define templates https://gist.github.com/kamaradclimber/9506031

@karmi
Copy link
Contributor

karmi commented Jun 18, 2014

@kamaradclimber I've went through our discussion again, and I somehow missed one issue you've mentioned -- the custom configuration.

This is in fact supported by the cookbook for a long time, see:

This doesn't address your issue?

@karmi karmi added the waiting label Jun 18, 2014
@kamaradclimber
Copy link
Author

see my comment on Feb 15

@karmi
Copy link
Contributor

karmi commented Jun 18, 2014

Yeah, right, so it works for normal attributes, but not when you wanna drop some custom YAML into the file...

I'm afraid I still think this is something I don't want to encourage. I'd like people to configure index mappings on the application level, not by a provisioning system.

Recently, I've added support for customizing the source for the .yml files in #153, which could make it easier for you to use a different template, with different configuration.

@kamaradclimber
Copy link
Author

I agree with you on the need to encourage index conf to be done elsewhere.

Since I am using this commit only to configure http.compression and …cluster_concurrent_rebalance, I think we don' t need it anymore.

(The configurable source would also be enough)

@karmi
Copy link
Contributor

karmi commented Jun 18, 2014

Great!, let's close this issue then, all right?

@karmi karmi closed this Jun 18, 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.

2 participants