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

Bump version to 2.1.0.0 #1883

Merged
merged 2 commits into from
Jun 16, 2022
Merged

Conversation

cliu123
Copy link
Member

@cliu123 cliu123 commented Jun 10, 2022

Description

[Describe what this change achieves]

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation) Test fix
  • Why these changes are required?
  • What is the old behavior before changes and new behavior after changes?

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

  • Commits are signed per the DCO using --signoff

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.

Signed-off-by: cliu123 <lc12251109@gmail.com>
Signed-off-by: cliu123 <lc12251109@gmail.com>
@cliu123 cliu123 changed the title Rebump to 2.1.0 Bump version to 2.1.0.0 Jun 10, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2022

Codecov Report

Merging #1883 (f01b25b) into main (001d73f) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@             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     
Impacted Files Coverage Δ
...security/configuration/DlsFlsFilterLeafReader.java 61.59% <0.00%> (-0.15%) ⬇️
...org/opensearch/security/rest/TenantInfoAction.java 77.94% <0.00%> (-10.30%) ⬇️
...security/auditlog/sink/ExternalOpenSearchSink.java 59.25% <0.00%> (-2.47%) ⬇️
...search/security/auditlog/impl/RequestResolver.java 69.88% <0.00%> (+2.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 001d73f...f01b25b. Read the comment docs.

@cliu123 cliu123 marked this pull request as ready for review June 10, 2022 22:01
@cliu123 cliu123 requested a review from a team June 10, 2022 22:01
@@ -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,
Copy link
Member Author

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);
Copy link
Member Author

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.

Copy link
Member

@peternied peternied left a 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.

@cliu123
Copy link
Member Author

cliu123 commented Jun 13, 2022

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(BWC Version test matrix) on the requirements for BWC tests:

At the time of this writing there are OpenSearch releases for 1.0, 1.1, 1.2, 1.3, and 2.0. To ensure bwc tests have appropriate coverage when building components for 2.1, the bwc test matrix should include 1.3, and 2.0.

Was there any discussion on this topic? I don't know any plugins plan to do this though.
Cc: @CEHENKLE @bbarani @VachaShah @saratvemulapalli @ohltyler for inputs.

@saratvemulapalli
Copy link
Member

@cliu123 we do not mandate this yet but that is our recommendation to test these versions.
There was a discussion to clarify this: opensearch-project/OpenSearch#3440

@cliu123
Copy link
Member Author

cliu123 commented Jun 13, 2022

@saratvemulapalli Thanks for fill me in the context!
@peternied Looks like we don't have to block the version bump on this as this is not mandated yet. We can re-visit this in a separate PR/issue if this will turn to a mandated global campaign across the entire OpenSearch org.

@cliu123 cliu123 requested review from peternied and a team June 13, 2022 21:28
@saratvemulapalli
Copy link
Member

@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.

@peternied
Copy link
Member

@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?

@cliu123
Copy link
Member Author

cliu123 commented Jun 13, 2022

@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.
If necessary, let's track this with a separate issue. We can get inputs from @VachaShah and other people who have more context on the BWC test framework and see how this should be done more generically.

@peternied
Copy link
Member

With the design of the BWC test framework , supporting multiple upgrade paths would require significant changes.

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.

@cliu123
Copy link
Member Author

cliu123 commented Jun 14, 2022

@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.
@VachaShah Do you happen to have answer for @peternied 's question above as you have the most context on the BWC test framwork? I don't see any way to support multiple upgrade paths without creating more test clusters, and I believe it would require more changes than that. If there's a generic solution for this that can be applied to all OpenSearch components, would you please provide? Thanks!

Copy link
Member

@peternied peternied left a 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.

@cliu123 cliu123 merged commit cd28b11 into opensearch-project:main Jun 16, 2022
@cliu123 cliu123 deleted the rebump_to_2.1.0 branch June 16, 2022 17:08
@cliu123
Copy link
Member Author

cliu123 commented Jun 16, 2022

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

stephen-crawford pushed a commit to stephen-crawford/security that referenced this pull request Nov 10, 2022
Signed-off-by: cliu123 <lc12251109@gmail.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023
Signed-off-by: cliu123 <lc12251109@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants