-
Notifications
You must be signed in to change notification settings - Fork 168
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
Ignore name of index when fetching settings #823
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #823 +/- ##
==========================================
- Coverage 71.95% 70.29% -1.66%
==========================================
Files 91 113 +22
Lines 8001 8881 +880
==========================================
+ Hits 5757 6243 +486
- Misses 2244 2638 +394 ☔ View full report in Codecov by Sentry. |
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.
What's the value of _name
vs. what it is after the change here?
Can you please add a test?
263b708
to
2ed7b6e
Compare
Thank you @dblock for your quick response! The value of removing If it's wanted, I could add a check that the alias only points to one index to raise a more specific error. Would that be appreciated? I've added a test, but I'm a little skeptical of the coverage report as it's based off a 6-month-old commit. Thank you! |
Possibly, but doesn't have to block this PR. Give it a shot after I merge this! |
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.
This all makes sense.
I think you have to do the same for the async code (see https://github.com/opensearch-project/opensearch-py/blob/main/opensearchpy/_async/helpers/index.py#L308). Add a test for that too.
e026aaf
to
50771c1
Compare
This commit fixes a situation where an index cannot be updated through this client as the index is aliased, with the client pointing to the alias. As the `GET /<index>/_settings` request will only ever return the settings for the specified index (through the alias), it would only have one key, so the name of the key would not matter. We can pop the key to get the settings object for the index through the alias. Signed-off-by: Étienne Beaulé <beauleetienne0@gmail.com>
I've added support for async, the checks for multiple indices, and testing for these. |
Description
This commit fixes a situation where an index cannot be updated through this client as the index is aliased, with the client pointing to the alias.
As the
GET /<index>/_settings
request will only ever return the settings for the specified index (through the alias), it would only have one key, so the name of the key would not matter. We can pop the key to get the settings object for the index through the alias.Describe what this change achieves.
Issues Resolved
Closes #822
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.