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

Fixes logic error in index settings #593

Merged
merged 4 commits into from
Nov 2, 2017
Merged

Conversation

yahooguntu
Copy link

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):

  • reset_disable_refresh_interval
  • reset_no_replicas

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
@pyromaniac
Copy link
Contributor

pyromaniac commented Oct 30, 2017

Hey, great finding! Can you add a spec for the case, please?

@yahooguntu yahooguntu closed this Nov 1, 2017
@yahooguntu yahooguntu reopened this Nov 1, 2017
@yahooguntu
Copy link
Author

@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({})
Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

@pyromaniac pyromaniac left a 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

@yahooguntu yahooguntu closed this Nov 2, 2017
@yahooguntu yahooguntu reopened this Nov 2, 2017
@pyromaniac pyromaniac merged commit 71043aa into toptal:master Nov 2, 2017
@pyromaniac
Copy link
Contributor

Thanks for the contribution!

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