Switch to standard OpenSearch gradle build #1888
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #1888 +/- ##
============================================
- Coverage 61.04% 61.01% -0.03%
- Complexity 3234 3235 +1
============================================
Files 256 256
Lines 18088 18088
Branches 3224 3224
============================================
- Hits 11041 11036 -5
- Misses 5467 5468 +1
- Partials 1580 1584 +4
Continue to review full report at Codecov.
|
cliu123
reviewed
Jun 16, 2022
cliu123
reviewed
Jun 16, 2022
cliu123
previously requested changes
Jun 16, 2022
There was a problem hiding this comment.
These are very sensitive changes. We need to make sure not to change any dependencies, artifacts, tests etc. Any miss could introduce build, test and functionality regressions. A couple of initial comments:
- There are only 245 tests running:
245 tests completed, 1 failed, 8 skipped
Before the change, 1111 tests running:
1111 tests completed, 1 failed, 46 skipped - Please verify all the dependencies and artifacts being generated are exactly same.
See in the description
Great idea to check this... I'll see about getting before/after test lists. |
I see some differences here: |
|
Suspicious that a significant portion of the coverage drop comes from whitelist-named files 🤔 |
|
@cliu123 Thanks for checking in on those differences, they are all expected, here is a quick rundown with explainations for each. The jackson-databind version is in now in alignment with the OpenSearch repository, this difference is expected The plugin descriptor follows the template pattern, see expanded diff
The changes within the opensearch-security-*.jar are ignorable,
Inside opensearch-security-*.jar, updated META-INF\MANIFEST.MF file
Inside opensearch-security-*.jar, deleted git.properties. The manifest includes the relevant details and removes the custom build logic to generate this
|
The logic and tests for whitelist have not been removed. It's been deprecated, but not removed. Allowlist was introduced, but it included both logic and tests, so it didn't have much impact on the test coverage. |
|
@peternied Great work! Could you move the changes that are not required to standardize the build with OpenSearch plugin? Things like test refactoring, sun_check(non-inclusive terminologies) etc. are not in the scope of this PR. They can be in separate PRs. |
|
@cliu123 I was able to remove the checkstyle supressions, but the other changes are needed. Please create comments on individual changes if you think they should be removed. Calling out the test changes, they are needed because the Parameterized runner cannot be used with RandomizedTest runner, this was preventing these tests from being executed |
|
@peternied Can you please review the failing CI build for java-17? |
* Rewrote build.gradle to follow OpenSearch plugin standards * Enabled license headers check on the repository * Did not enable several new repository checks * Added maven publishing for security * Removed excess forced dependency resolutions * Rebuilt projects dependencies, release plugin binary has the exact same dependenices and versions * Converted dependencies into runtime dependencies to avoid use during coding * Converted dependencies from project wide to test only dependencies * jackson-databind version comes from OpenSearch * Replaced handmade build manifest with git properties to automated version used by OpenSearch * Added license headers to files that were missing them * Checkstyle improvements * Removed check for one-off issue with incorrect headers * Disable checkstyle checks that are not errors * Added comment for checkstyle supression * Moved checkstyle file into directory off of the project root * Moved standard configuration directory * Using default test running from OpenSearch * Switched to RandomizedTest as the base test class * Parameterized test runner cannot be used with RandomizedTest coverted test functionality * Fixed tests resource issues, added new fields to make this consistant in the codebase * Fixed issue with leaky environment variable for security root directory, refactored usage * Removed unneeded setDefaultUncaughtExceptionHandler in tests * Fixed issues with deprecated internal reflection from Mockito * Disabled ThreadLeak detection as it is catching real issues that are mitigated by retries * Initial pass on non-inclusive terminology, commented out exclusions * Removed test dependency on scala * Removed test dependency on legacy xmlsecurity library Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
…t cases Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
DarshitChanpura
approved these changes
Jun 23, 2022
@peternied I know that @vrozov mentioned that Randomized Runner could introduce some regressions for some reason, but @vrozov has more context on this. Would you follow up? |
|
I see some test failures in CI. And 1147 tests ran, but only 1111 tests ran before the changes as I commented above. The build system migration change should not add more tests though. |
When I compare the number of tests run against main I see 1146, where as with this change I see 1145. The number of tests includes retries, so exact numeric matches aren't. That aside, the associated test coverage is nearly identical. I don't think there is an issue here - how does this sound to you? |
davidlago
approved these changes
Jun 24, 2022
There was a problem hiding this comment.
At this point, I don't think we have enough solid information to block/delay this PR on the suspicion that the Randomized Runner might be causing issues. I support moving forward with this change and addressing any particular issues (if/when confirmed) as they arise. This is an important PR that is long overdue to bring us in line with the rest of the project.
|
I've dismissed my review. |
cliu123
reviewed
Jun 24, 2022
src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java
Show resolved
Hide resolved
Thanks for clarifying! Looking at the test summary, I see more retries, which means more tests have become flaky(6 tests failed). I'd suggest to re-run CI a few times to confirm the stability of tests remain same. |
cliu123
reviewed
Jun 24, 2022
cliu123
reviewed
Jun 24, 2022
src/test/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticatorTest.java
Show resolved
Hide resolved
cliu123
reviewed
Jun 24, 2022
cliu123
reviewed
Jun 24, 2022
cliu123
reviewed
Jun 24, 2022
.../java/org/opensearch/security/dlic/dlsfls/DlsFlsCrossClusterMinimalRoundtripSearchTests.java
Show resolved
Hide resolved
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Jun 24, 2022
* Switch to standard OpenSearch gradle build * Rewrote build.gradle to follow OpenSearch plugin standards * Enabled license headers check on the repository * Did not enable several new repository checks * Added maven publishing for security * Removed excess forced dependency resolutions * Rebuilt projects dependencies, release plugin binary has the exact same dependencies and versions * Converted dependencies into runtime dependencies to avoid use during coding * Converted dependencies from project wide to test only dependencies * jackson-databind version comes from OpenSearch * Replaced handmade build manifest with git properties to automated version used by OpenSearch * Added license headers to files that were missing them * Checkstyle improvements * Disable checkstyle checks that are not errors * Moved checkstyle file into directory off of the project root * Moved standard configuration directory * Using default test running from OpenSearch * Switched to RandomizedTest as the base test class * Parameterized test runner cannot be used with RandomizedTest converted test functionality * Fixed tests resource issues, added new fields to make this consistent in the codebase * Fixed issue with leaky environment variable for security root directory, refactored usage * Removed unneeded setDefaultUncaughtExceptionHandler in tests * Fixed issues with deprecated internal reflection from Mockito * Disabled ThreadLeak detection as it is catching real issues that are mitigated by retries * Initial pass on non-inclusive terminology, commented out exclusions * Removed test dependency on scala * Removed test dependency on legacy xmlsecurity library Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit 03a224d)
peternied
added a commit
that referenced
this pull request
Jun 24, 2022
* Switch to standard OpenSearch gradle build (#1888) * Rewrote build.gradle to follow OpenSearch plugin standards * Enabled license headers check on the repository * Did not enable several new repository checks * Added maven publishing for security * Removed excess forced dependency resolutions * Rebuilt projects dependencies, release plugin binary has the exact same dependencies and versions * Converted dependencies into runtime dependencies to avoid use during coding * Converted dependencies from project wide to test only dependencies * jackson-databind version comes from OpenSearch * Replaced handmade build manifest with git properties to automated version used by OpenSearch * Added license headers to files that were missing them * Checkstyle improvements * Disable checkstyle checks that are not errors * Moved checkstyle file into directory off of the project root * Moved standard configuration directory * Using default test running from OpenSearch * Switched to RandomizedTest as the base test class * Parameterized test runner cannot be used with RandomizedTest converted test functionality * Fixed tests resource issues, added new fields to make this consistent in the codebase * Fixed issue with leaky environment variable for security root directory, refactored usage * Removed unneeded setDefaultUncaughtExceptionHandler in tests * Fixed issues with deprecated internal reflection from Mockito * Disabled ThreadLeak detection as it is catching real issues that are mitigated by retries * Initial pass on non-inclusive terminology, commented out exclusions * Removed test dependency on scala * Removed test dependency on legacy xmlsecurity library Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit 03a224d) * Fix build break from cluster manager changes Signed-off-by: Peter Nied <petern@amazon.com> Co-authored-by: Peter Nied <petern@amazon.com>
3 tasks
stephen-crawford
pushed a commit
to stephen-crawford/security
that referenced
this pull request
Nov 10, 2022
…-project#1903) * Switch to standard OpenSearch gradle build (opensearch-project#1888) * Rewrote build.gradle to follow OpenSearch plugin standards * Enabled license headers check on the repository * Did not enable several new repository checks * Added maven publishing for security * Removed excess forced dependency resolutions * Rebuilt projects dependencies, release plugin binary has the exact same dependencies and versions * Converted dependencies into runtime dependencies to avoid use during coding * Converted dependencies from project wide to test only dependencies * jackson-databind version comes from OpenSearch * Replaced handmade build manifest with git properties to automated version used by OpenSearch * Added license headers to files that were missing them * Checkstyle improvements * Disable checkstyle checks that are not errors * Moved checkstyle file into directory off of the project root * Moved standard configuration directory * Using default test running from OpenSearch * Switched to RandomizedTest as the base test class * Parameterized test runner cannot be used with RandomizedTest converted test functionality * Fixed tests resource issues, added new fields to make this consistent in the codebase * Fixed issue with leaky environment variable for security root directory, refactored usage * Removed unneeded setDefaultUncaughtExceptionHandler in tests * Fixed issues with deprecated internal reflection from Mockito * Disabled ThreadLeak detection as it is catching real issues that are mitigated by retries * Initial pass on non-inclusive terminology, commented out exclusions * Removed test dependency on scala * Removed test dependency on legacy xmlsecurity library Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit 03a224d) * Fix build break from cluster manager changes Signed-off-by: Peter Nied <petern@amazon.com> Co-authored-by: Peter Nied <petern@amazon.com>
stephen-crawford
pushed a commit
to stephen-crawford/security
that referenced
this pull request
Nov 10, 2022
* Switch to standard OpenSearch gradle build * Rewrote build.gradle to follow OpenSearch plugin standards * Enabled license headers check on the repository * Did not enable several new repository checks * Added maven publishing for security * Removed excess forced dependency resolutions * Rebuilt projects dependencies, release plugin binary has the exact same dependencies and versions * Converted dependencies into runtime dependencies to avoid use during coding * Converted dependencies from project wide to test only dependencies * jackson-databind version comes from OpenSearch * Replaced handmade build manifest with git properties to automated version used by OpenSearch * Added license headers to files that were missing them * Checkstyle improvements * Disable checkstyle checks that are not errors * Moved checkstyle file into directory off of the project root * Moved standard configuration directory * Using default test running from OpenSearch * Switched to RandomizedTest as the base test class * Parameterized test runner cannot be used with RandomizedTest converted test functionality * Fixed tests resource issues, added new fields to make this consistent in the codebase * Fixed issue with leaky environment variable for security root directory, refactored usage * Removed unneeded setDefaultUncaughtExceptionHandler in tests * Fixed issues with deprecated internal reflection from Mockito * Disabled ThreadLeak detection as it is catching real issues that are mitigated by retries * Initial pass on non-inclusive terminology, commented out exclusions * Removed test dependency on scala * Removed test dependency on legacy xmlsecurity library Signed-off-by: Peter Nied <petern@amazon.com> Signed-off-by: Stephen Crawford <steecraw@amazon.com>
stephen-crawford
pushed a commit
to stephen-crawford/security
that referenced
this pull request
Nov 10, 2022
…-project#1903) * Switch to standard OpenSearch gradle build (opensearch-project#1888) * Rewrote build.gradle to follow OpenSearch plugin standards * Enabled license headers check on the repository * Did not enable several new repository checks * Added maven publishing for security * Removed excess forced dependency resolutions * Rebuilt projects dependencies, release plugin binary has the exact same dependencies and versions * Converted dependencies into runtime dependencies to avoid use during coding * Converted dependencies from project wide to test only dependencies * jackson-databind version comes from OpenSearch * Replaced handmade build manifest with git properties to automated version used by OpenSearch * Added license headers to files that were missing them * Checkstyle improvements * Disable checkstyle checks that are not errors * Moved checkstyle file into directory off of the project root * Moved standard configuration directory * Using default test running from OpenSearch * Switched to RandomizedTest as the base test class * Parameterized test runner cannot be used with RandomizedTest converted test functionality * Fixed tests resource issues, added new fields to make this consistent in the codebase * Fixed issue with leaky environment variable for security root directory, refactored usage * Removed unneeded setDefaultUncaughtExceptionHandler in tests * Fixed issues with deprecated internal reflection from Mockito * Disabled ThreadLeak detection as it is catching real issues that are mitigated by retries * Initial pass on non-inclusive terminology, commented out exclusions * Removed test dependency on scala * Removed test dependency on legacy xmlsecurity library Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit 03a224d) * Fix build break from cluster manager changes Signed-off-by: Peter Nied <petern@amazon.com> Co-authored-by: Peter Nied <petern@amazon.com> Signed-off-by: Stephen Crawford <steecraw@amazon.com>
stephen-crawford
pushed a commit
to stephen-crawford/security
that referenced
this pull request
Nov 10, 2022
…-project#1903) * Switch to standard OpenSearch gradle build (opensearch-project#1888) * Rewrote build.gradle to follow OpenSearch plugin standards * Enabled license headers check on the repository * Did not enable several new repository checks * Added maven publishing for security * Removed excess forced dependency resolutions * Rebuilt projects dependencies, release plugin binary has the exact same dependencies and versions * Converted dependencies into runtime dependencies to avoid use during coding * Converted dependencies from project wide to test only dependencies * jackson-databind version comes from OpenSearch * Replaced handmade build manifest with git properties to automated version used by OpenSearch * Added license headers to files that were missing them * Checkstyle improvements * Disable checkstyle checks that are not errors * Moved checkstyle file into directory off of the project root * Moved standard configuration directory * Using default test running from OpenSearch * Switched to RandomizedTest as the base test class * Parameterized test runner cannot be used with RandomizedTest converted test functionality * Fixed tests resource issues, added new fields to make this consistent in the codebase * Fixed issue with leaky environment variable for security root directory, refactored usage * Removed unneeded setDefaultUncaughtExceptionHandler in tests * Fixed issues with deprecated internal reflection from Mockito * Disabled ThreadLeak detection as it is catching real issues that are mitigated by retries * Initial pass on non-inclusive terminology, commented out exclusions * Removed test dependency on scala * Removed test dependency on legacy xmlsecurity library Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit 03a224d) * Fix build break from cluster manager changes Signed-off-by: Peter Nied <petern@amazon.com> Co-authored-by: Peter Nied <petern@amazon.com> Signed-off-by: Stephen Crawford <steecraw@amazon.com>
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.3 1.3
# Navigate to the new working tree
cd .worktrees/backport-1.3
# Create a new branch
git switch --create backport/backport-1888-to-1.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 03a224d16045a8f561e2d7d84b8765c95309524c
# Push it to GitHub
git push --set-upstream origin backport/backport-1888-to-1.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.3Then, create a pull request where the |
wuychn
pushed a commit
to ochprince/security
that referenced
this pull request
Mar 16, 2023
…-project#1903) * Switch to standard OpenSearch gradle build (opensearch-project#1888) * Rewrote build.gradle to follow OpenSearch plugin standards * Enabled license headers check on the repository * Did not enable several new repository checks * Added maven publishing for security * Removed excess forced dependency resolutions * Rebuilt projects dependencies, release plugin binary has the exact same dependencies and versions * Converted dependencies into runtime dependencies to avoid use during coding * Converted dependencies from project wide to test only dependencies * jackson-databind version comes from OpenSearch * Replaced handmade build manifest with git properties to automated version used by OpenSearch * Added license headers to files that were missing them * Checkstyle improvements * Disable checkstyle checks that are not errors * Moved checkstyle file into directory off of the project root * Moved standard configuration directory * Using default test running from OpenSearch * Switched to RandomizedTest as the base test class * Parameterized test runner cannot be used with RandomizedTest converted test functionality * Fixed tests resource issues, added new fields to make this consistent in the codebase * Fixed issue with leaky environment variable for security root directory, refactored usage * Removed unneeded setDefaultUncaughtExceptionHandler in tests * Fixed issues with deprecated internal reflection from Mockito * Disabled ThreadLeak detection as it is catching real issues that are mitigated by retries * Initial pass on non-inclusive terminology, commented out exclusions * Removed test dependency on scala * Removed test dependency on legacy xmlsecurity library Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit 03a224d) * Fix build break from cluster manager changes Signed-off-by: Peter Nied <petern@amazon.com> Co-authored-by: Peter Nied <petern@amazon.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
By switching to the standard build process we should no longer have to solve build related issues in isolation, we should be able to reuse all the standard scripts and practices by the OpenSearch organization.
Issues Resolved
Testing
Security plugin - comparison
The only meaningful difference seen here is the jackson-databind version, which is in alignment with the OpenSearch repository.
Check List
New functionality includes testingNew functionality has been documentedBy 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.