-
Couldn't load subscription status.
- Fork 808
[MODEL] Load settings from a yaml file #346
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
Conversation
This change allows you to specify a YAML file with settings for the indices. It would be helpful if you want to reuse analyzers in many different indexes. Also, YAML is much more readable than hashes in ruby code. Example: # config/elasticsearch/custom_analyzers.yml # # number_of_shards: 5 # analysis: # analyzer: # my_custom_analyzer: # tokenizer: "whitespace" # filter: ["lowercase", "asciifolding"] # class Article include Elasticsearch::Model settings "config/elasticsearch/custom_analyzers.yml" end class Author include Elasticsearch::Model settings "config/elasticsearch/custom_analyzers.yml" end
Hello David, thanks for the patch!
I must say that I'm quite against loading configs from YAML files :), despite the prevalence in the Rails world. On the other hand, I like the use case you have in mind -- specifying analyzers and other index settings.
I think we can take this even further and allow both .yml and .json files as an index specification, and include instructions how to specify the settings in plain Ruby. I'll leave a comment on the specific lines in the source.
I'll be offline for the next two weeks, so please ping me when I forget to return to this after I'm back online.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can improve this significantly by checking whether the argument is an IO object, ie. when it responds to read. This would not only make testing easier, but would allow people to use the feature in a much more interesting way -- the settings then don't have to come from a physical file on disk, but from Elasticsearch itself, from an API, wrapped in StringIO, etc etc etc. What do you think?
I'd also like to allow both .yml and .json files here, just to have parity with Elasticsearch itself (we can always check the extension when the argument is a physical file with a name).
What do you think? Is it something you'd like to take on or should I take it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll make the changes and submit them. Thanks!
I'm also looking around for the best way to do it for mapping. In my apps I currently configure the whole index, including mappings, via a YAML file by patching the gem and adding a whole "configure_from_file" method that simply reads it and send it as the body of the index at creation. I don't know if you go that far for the gem. For now settings would help with the whole repeated analyzers thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an example here: https://github.com/elastic/elasticsearch-rails/blob/master/elasticsearch-persistence/examples/music/index_manager.rb, where the IndexManager class simply gathers and merges settings/mappings from a model, and creates an index.
I feel like this is something best left to the application developer, simply because there are dozens of ways how to do this (simply storing a Hash in some global module, having a module method, putting it in YAML and reading it, using an existing gem for that, ...) -- but all that said I like the feature you propose. Let's first have that, document it, maybe use it in our example applications and then discuss if we can put some more sugar on it?
Closed in favor of #351. Decided to go with Convention over Configuration and simply load the files from config/elasticsearch named after the document_type of the class. I think its simpler and easier to use.
Still preferring this over #351 :) Let's keep this one open for additions and changes (just add commits and force push the branch, either me or you can squash+rebase when we're ready to merge).
I'll work on it within the next week or so 👍
Great news, please ping me when you're ready, multiple times if needed :)
@karmi Actually decided to work on it now, let me know if this is what you meant or need any other change.
@dabit Yes, this is still good to me!, if you address the comments I did over the patch (mainly passing the IO object if I remember correctly)
@karmi Ok, now I'll work on the sample app, see what we get from there.
@karmi Forgot to mention that I updated the sample app, think you can take a look and see if this is what you meant?
Thanks!
... or JSON file This change allows you to specify a YAML file with settings for the indices. It would be helpful if you want to reuse analyzers in many different indexes. Also, YAML is much more readable than hashes in ruby code. Example: # config/elasticsearch/custom_analyzers.yml # # number_of_shards: 5 # analysis: # analyzer: # my_custom_analyzer: # tokenizer: "whitespace" # filter: ["lowercase", "asciifolding"] # class Article include Elasticsearch::Model settings "config/elasticsearch/custom_analyzers.yml" end class Author include Elasticsearch::Model settings "config/elasticsearch/custom_analyzers.yml" end Closes: #346 Closes: #351
Hi David, finally found time to check & merge this. Many thanks!!!
Closed in 5c7cd12
Thanks!
This change allows you to read settings from any object that responds to :read, like a
file with YAML or JSON.
It would be helpful if you want to reuse analyzers in many different
indexes.
Example: