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

updated compatibility information #189

Merged

Conversation

AbhinavGarg90
Copy link
Contributor

Description

updates comments regarding the compatibility policy to reflect that the client does not need to remain in major version lockstep with the server.

Issues Resolved

#178

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.

added row on table relating to the 2.x version of the client
added updated policy information regarding compatibility

Signed-off-by: Abhinav Garg <[email protected]>
opensearch/src/lib.rs Outdated Show resolved Hide resolved
@reta reta added the skip-changelog Skip changelog verification label Oct 4, 2023
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #189 (c14dcce) into main (94e7b72) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head c14dcce differs from pull request most recent head 597590c. Consider uploading reports for the commit 597590c to get more accurate results

@@           Coverage Diff           @@
##             main     #189   +/-   ##
=======================================
  Coverage   73.82%   73.82%           
=======================================
  Files         402      401    -1     
  Lines       63738    63736    -2     
=======================================
  Hits        47055    47055           
+ Misses      16683    16681    -2     
Flag Coverage Δ
integration 73.82% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 2 files with indirect coverage changes

dblock
dblock previously requested changes Oct 4, 2023
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see tests in this PR run against OpenSearch 1.x and 2.x, so is this correct? AFAIK 2.x client is compatible with both OpenSearch 1.x and 2.x.

Co-authored-by: Andriy Redko <[email protected]>
Signed-off-by: Abhinav Garg <[email protected]>
@AbhinavGarg90
Copy link
Contributor Author

I see tests in this PR run against OpenSearch 1.x and 2.x, so is this correct? AFAIK 2.x client is compatible with both OpenSearch 1.x and 2.x.

Could you please elaborate? What action is needed from my end?

@Xtansia
Copy link
Collaborator

Xtansia commented Oct 8, 2023

I see tests in this PR run against OpenSearch 1.x and 2.x, so is this correct? AFAIK 2.x client is compatible with both OpenSearch 1.x and 2.x.

Mostly yes, though 2.x of the client dropped the "types" apis, so not 100% compatible with 1.x.

@dblock
Copy link
Member

dblock commented Oct 9, 2023

I see tests in this PR run against OpenSearch 1.x and 2.x, so is this correct? AFAIK 2.x client is compatible with both OpenSearch 1.x and 2.x.

Could you please elaborate? What action is needed from my end?

The line that says | 2.x | 2.x | is incorrect. It should be | 1.x - 2.x | 2.x | or something like that?

@dblock
Copy link
Member

dblock commented Oct 9, 2023

I see tests in this PR run against OpenSearch 1.x and 2.x, so is this correct? AFAIK 2.x client is compatible with both OpenSearch 1.x and 2.x.

Mostly yes, though 2.x of the client dropped the "types" apis, so not 100% compatible with 1.x.

I see. So do you think the code in this PR is correct? If so, then let's merge?

@Xtansia
Copy link
Collaborator

Xtansia commented Oct 11, 2023

I see tests in this PR run against OpenSearch 1.x and 2.x, so is this correct? AFAIK 2.x client is compatible with both OpenSearch 1.x and 2.x.

Mostly yes, though 2.x of the client dropped the "types" apis, so not 100% compatible with 1.x.

I see. So do you think the code in this PR is correct? If so, then let's merge?

It's tricky as only listing 2.x as compatible definitely undersells the level of compatibility, but we also probably don't want to list 1.x without qualifying conditions in case we get complaints around missing functionality for 1.x.

But maybe we can do something like:


Rust client OpenSearch
1.x 1.x
2.x 2.x, 1.x^
  • ^: With the exception of some previously deprecated APIs

@AbhinavGarg90
Copy link
Contributor Author

I see tests in this PR run against OpenSearch 1.x and 2.x, so is this correct? AFAIK 2.x client is compatible with both OpenSearch 1.x and 2.x.

Mostly yes, though 2.x of the client dropped the "types" apis, so not 100% compatible with 1.x.

I see. So do you think the code in this PR is correct? If so, then let's merge?

It's tricky as only listing 2.x as compatible definitely undersells the level of compatibility, but we also probably don't want to list 1.x without qualifying conditions in case we get complaints around missing functionality for 1.x.

But maybe we can do something like:

Rust client OpenSearch
1.x 1.x
2.x 2.x, 1.x^

  • ^: With the exception of some previously deprecated APIs

this sounds like a good idea. Gives a fair warning to somebody, but still ensure they know that it is compatible to an extent. Should I commit that change?

@AbhinavGarg90
Copy link
Contributor Author

the additional line has been added. Does that look fine @Xtansia ?

Copy link
Collaborator

@Xtansia Xtansia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a minor typo to fix please @AbhinavGarg90.

Thanks!

opensearch/src/lib.rs Outdated Show resolved Hide resolved
fixed typo

Co-authored-by: Thomas Farr <[email protected]>
Signed-off-by: Abhinav Garg <[email protected]>
@Xtansia Xtansia dismissed dblock’s stale review October 17, 2023 20:04

2.x's compatibility with 1.x server has been included in the comment

@Xtansia Xtansia merged commit 13d0d4b into opensearch-project:main Oct 17, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OSCI-2023 skip-changelog Skip changelog verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants