-
Notifications
You must be signed in to change notification settings - Fork 59
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
Support inclusive #65
Conversation
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.
Thank you @shyim for adding these changes!
There are some more references in https://github.com/opensearch-project/opensearch-php/blob/main/tests/Connections/ConnectionTest.php. Can we replace them as well?
Sure I will look later :) |
6e25b66
to
4c4beb1
Compare
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.
Thank you @shyim!
@dblock @saratvemulapalli review please? |
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.
Will this code stop working against older versions of OpenSearch? It looks like it. In order to ensure the client is compatible with at least two major versions and thus allow a migration path, I think we need to deprecate functionality in 1 major release, then remove it in the next major release. So I think in this change we cannot throw errors, we need to log a warning for the deprecation.
1baf3d8
to
5c60d7c
Compare
Signed-off-by: Soner Sayakci <[email protected]>
Signed-off-by: Soner Sayakci <[email protected]>
Signed-off-by: Soner Sayakci <[email protected]>
@shyim See my question above though, what's the definite answer? |
@dblock ohh I didn't see that. The old parameter will be still send, but throws just a deprecation with the replacement :) |
Ok, I think this works. |
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.
LGTM! Thank you @shyim
* feat: add cluster manager api Signed-off-by: Soner Sayakci <[email protected]> * feat: deprecate master_timeout Signed-off-by: Soner Sayakci <[email protected]> * feat: update all comments Signed-off-by: Soner Sayakci <[email protected]> Signed-off-by: Soner Sayakci <[email protected]> Signed-off-by: joanna.bak <[email protected]>
Description
Add inclusive names
Issues Resolved
#61
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.