-
Notifications
You must be signed in to change notification settings - Fork 368
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
Fixes logic error in index settings #593
Conversation
Only occurs if you have these two settings enabled (and probably any other config option that saves/restores index settings): * reset_disable_refresh_interval * reset_no_replicas
Hey, great finding! Can you add a spec for the case, please? |
@pyromaniac How's that look? |
|
||
specify 'uses empty index settings if not defined' do | ||
allow(Chewy).to receive(:wait_for_status).and_return(nil) | ||
allow(CitiesIndex).to receive(:settings_hash).and_return({}) |
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.
Did you check if it fails without your fix?
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.
Yeah, the root cause was because of spec/spec_helper.rb:27 ; if the :index option isn't set, then the tests fail because Index#index_settings tries to access 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.
Looks great, but can you get rid of byebug please? I don't think we need it
This reverts commit 536b147.
Thanks for the contribution! |
There's a logic error in the code that extracts an index's settings from the settings hash.
Only occurs if you have these two settings enabled (and probably any
other config option that saves/restores index settings):