-
Notifications
You must be signed in to change notification settings - Fork 3
Added the ability to define custom settings and mappings in a config #10
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
# Conflicts: # test/elastic_sync/schema_test.exs
lib/elastic_sync/index.ex
Outdated
names | ||
|> API.index | ||
|> HTTP.put | ||
|> HTTP.put(Tirexs.get_uri_env(), config) |
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.
is the Tirexs.get_uri_env
necessary?
lib/elastic_sync/index.ex
Outdated
|
||
def transition(name, fun) do | ||
transition(name, get_new_alias_name(name), fun) | ||
def transition(name, fun, config \\ []) do |
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.
we should be consistent with the type we're expecting for config
. Is it a keyword list or a map? Technically tirexs will accept both, but I'd actually prefer we consistently default to a map.
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.
config should be the second arg. the function should be the last.
i don't think we need to provide a default here. especially since I plan to make all of these functions take a struct later anyway.
lib/elastic_sync/schema.ex
Outdated
{mod, fun} = Keyword.get(opts, :config, {ElasticSync.Schema, :default_config}) | ||
|
||
validate_index_name!(index) | ||
validate_config!(mod, fun) |
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.
Since it isn't a required option, and since developers will just be entering raw values (not some computed value), I don't think we need to check for nil.
This looks great. I just have a few minor suggestions. Then, once CI is passing, this should be good to go! |
I'm good with this as soon as CI passes 👍 |
At last! |
you can now define settings and mappings in a function that will get plumbed down to the creation of the elastic search index
As talked about in #5