-
Notifications
You must be signed in to change notification settings - Fork 280
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
Bump version to 2.1.0.0 #1883
Bump version to 2.1.0.0 #1883
Conversation
Signed-off-by: cliu123 <[email protected]>
Signed-off-by: cliu123 <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1883 +/- ##
============================================
- Coverage 61.01% 60.98% -0.04%
- Complexity 3232 3234 +2
============================================
Files 256 256
Lines 18085 18086 +1
Branches 3222 3222
============================================
- Hits 11034 11029 -5
- Misses 5469 5476 +7
+ Partials 1582 1581 -1
Continue to review full report at Codecov.
|
@@ -195,11 +195,11 @@ protected void checkGeneralAccess(int status, String username, String password) | |||
rh.sendAdminCertificate = sendAdminCertificate; | |||
} | |||
|
|||
protected String checkReadAccess(int status, String username, String password, String indexName, String type, | |||
protected String checkReadAccess(int status, String username, String password, String indexName, String actionType, |
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.
Fixing the naming here as OpenSearch 2.0+ supports actionType in the API path and dropped the support for type.
@@ -337,8 +337,6 @@ private void checkAllSfAllowed() throws Exception { | |||
rh.sendAdminCertificate = false; | |||
checkReadAccess(HttpStatus.SC_OK, "picard", "picard", "sf", "_doc", 1); | |||
checkWriteAccess(HttpStatus.SC_OK, "picard", "picard", "sf", "_doc", 1); | |||
// ES7 only supports one doc type, so trying to create a second one leads to 400 BAD REQUEST | |||
checkWriteAccess(HttpStatus.SC_BAD_REQUEST, "picard", "picard", "sf", "public", 1); |
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.
Removing this assertion to align with the behavior change in OpenSearch 2.0.1.
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 change drops BWC support for 1.3.0, we should instead add support for 2.0.0 - following the guidance from the bwc version test matrix.
@peternied I see that you made the changes(
Was there any discussion on this topic? I don't know any plugins plan to do this though. |
@cliu123 we do not mandate this yet but that is our recommendation to test these versions. |
@saratvemulapalli Thanks for fill me in the context! |
@cliu123 my take, I would rather add 1.3.x bwc support now rather than piling up the tech debt when opensearch-build mandates these versions. |
@cliu123 Could you attempt to support BWC tests for 1.3.0 and 2.0.0 and if there are substantive issues we can create an issue to follow up in a separate PR? |
@saratvemulapalli @peternied With the design of the BWC test framework , supporting multiple upgrade paths would require significant changes. There is not an available way as easy as to support build with multiple JDKs using matrix. I don't think mixing up these changes with the version bump is what we want to do. |
Could you quantify this statement with concrete issues? If we put off doing this work because it is hard, we should articulate this to the OpenSearch team so it can be solved for everyone. |
@peternied This PR is supposed to include only the version changes. Other changes in BWC tests if required should be in a separate PR. We can explore a solution to support multiple upgrade paths. It is not worthwhile to block a version bump for BWC test changes that are supposed to be looked into separately. IMO, things that haven't been well-defined shouldn't block another PR unless the PR being blocked has hard dependencies on that. |
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.
I've looked into what it will take to update the BWC tests and created an issue opensearch-project/OpenSearch#3607 to get the proper support built in at a framework level. Lets move forward with the change you have and until the framework supports this scenario we will need to operate outside the guidance.
Created a separate issue to track the pending item: #1889 |
Signed-off-by: cliu123 <[email protected]> Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: cliu123 <[email protected]>
Description
[Describe what this change achieves]
Is this a backport? If so, please add backport PR # and/or commits #
Testing
[Please provide details of testing done: unit testing, integration testing and manual testing]
Check List
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.