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

add copy_settings=true to url params #1379

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

genevera
Copy link

@genevera genevera commented Apr 8, 2019

From issue #1378, fixes #1375

Proposed Changes

  • adds params={"copy_settings": "true"} to self.client.indices.shrink call

@genevera genevera force-pushed the copy_settings_for_shrink_1378 branch 3 times, most recently from 5d2e1f5 to 5598248 Compare April 8, 2019 01:34
@untergeek
Copy link
Member

Sorry for the very late response. I've looked at what you've contributed, but it needs some work before it can be merged.

  1. It forces copy_settings: true whether you want it that way or not (it's not a setting). I'd like to see it be optional, with the default being false since that preserves the current default behavior.
  2. It appears that the params block is not allowed in API calls before 6.4.0, so you can't just send params='', you have to omit the argument params altogether. This could be accomplished with an if/else block right where the call is made.
  3. No tests cases to validate these settings. I can help you to write tests, if you are interested.

If you're not interested, I will eventually add these myself, via a different PR.

I'm willing to help you through these

@genevera
Copy link
Author

No worries! I'd be happy to fix this up myself. Thank you for the review!

@untergeek
Copy link
Member

Please rebase against the current master. There have been quite a few changes since this PR was submitted.

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.

Curator restore command doesn't take into account the rename pattern
2 participants