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

Downgrade Gradle version #1661

Merged
merged 7 commits into from
Mar 15, 2022
Merged

Conversation

peternied
Copy link
Member

@peternied peternied commented Mar 2, 2022

Description

BWC tooling is built with gradle 6, which has breaking changes that are
not compatible with gradle 7. In order to support BWC tests we need to
align with the OpenSearch's gradle version for the 1.3 release.

  • NOTE: Merging this change into main and backporting to 1.3 so for the 2.0 release we will be at the same cadence as other plugins to avoid merge conflicts

Issues Resolved

Check List

  • New functionality includes testing
  • New functionality has been documented
  • 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.

BWC tooling is built with gradle 6, which has breaking changes that are
not compatiable with gradle 7.  In order to support BWC tests we need to
align with the OpenSearch's gradle version for the 1.3 release.

See Also:
* Gradle 7 PR in OpenSearch opensearch-project/OpenSearch#1622
* Distribution build bugs encountered by plugins opensearch-project/opensearch-build#1247
* Revert of Gradle 7 PR in OpenSearch  opensearch-project/OpenSearch#1657

Signed-off-by: Peter Nied <[email protected]>
@peternied peternied added v1.3.0 backport 1.3 backport to 1.3 branch labels Mar 2, 2022
@peternied peternied requested a review from a team March 2, 2022 21:13
@peternied peternied self-assigned this Mar 2, 2022
@peternied
Copy link
Member Author

Big thanks for @VachaShah for setting me on this path to get the BWC tests working

davidlago
davidlago previously approved these changes Mar 2, 2022
@cliu123
Copy link
Member

cliu123 commented Mar 2, 2022

Do we want to downgrade Gradle in both 1.3 and 2.0?

@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2022

Codecov Report

Merging #1661 (3de18de) into main (920701e) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 3de18de differs from pull request most recent head e825c26. Consider uploading reports for the commit e825c26 to get more accurate results

@@             Coverage Diff              @@
##               main    #1661      +/-   ##
============================================
+ Coverage     62.92%   62.94%   +0.02%     
- Complexity     3259     3261       +2     
============================================
  Files           253      253              
  Lines         18127    18126       -1     
  Branches       3258     3258              
============================================
+ Hits          11406    11410       +4     
+ Misses         5072     5067       -5     
  Partials       1649     1649              
Impacted Files Coverage Δ
...ecurity/ssl/rest/SecuritySSLReloadCertsAction.java 84.78% <0.00%> (-0.33%) ⬇️
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java 57.25% <0.00%> (+0.76%) ⬆️
...ecurity/configuration/ConfigurationRepository.java 75.27% <0.00%> (+2.19%) ⬆️

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 920701e...e825c26. Read the comment docs.

@peternied
Copy link
Member Author

peternied commented Mar 3, 2022

Do we want to downgrade Gradle in both 1.3 and 2.0?

@cliu123 Yes - there are gradle files for tests that need to be migrated to gradle 7 before they can be pulled in. This will keep the 2.0 branch stable until those dependent changes have been done.

@peternied
Copy link
Member Author

@opensearch-project/security Could I get another look at this PR?

davidlago
davidlago previously approved these changes Mar 4, 2022
@@ -16,11 +16,19 @@ jobs:

steps:

- name: Set up JDK
- name: Set up JDK for build and test
if: matrix.jdk != 17
Copy link
Member

Choose a reason for hiding this comment

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

Does JDK17 not support Gradle 6.9?

Copy link
Member Author

Choose a reason for hiding this comment

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

No it does not

Copy link
Member Author

@peternied peternied Mar 4, 2022

Choose a reason for hiding this comment

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

Following the example from opensearch-project/common-utils#121

@@ -224,6 +224,21 @@ test {
maxFailures = 30
maxRetries = 5
}
jacoco {
excludes = [
Copy link
Member

Choose a reason for hiding this comment

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

Why these dependencies need to be excluded from Jacoco?

Copy link
Member Author

@peternied peternied Mar 4, 2022

Choose a reason for hiding this comment

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

In JDK17 these class files are not instrumentable (I don't have the background just dealt with the error), had to remove them to prevent test failures.

None of these exclusions are classes that we implement in our code, and thus do not need to be instrumented for our coverage statistics

@peternied
Copy link
Member Author

This change also resolves #1502

@peternied
Copy link
Member Author

@opensearch-project/security Can I get another look at this PR?

@peternied
Copy link
Member Author

This missed the 1.3 release, removed those labels. @DarshitChanpura @cliu123 please review and provide feedback so we can move forward with this PR

@peternied
Copy link
Member Author

Addressing the @whitesource-for-github-com WhiteSource Security Check Failing in PR #1679

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

White-source check fails can you please check it?

@peternied
Copy link
Member Author

White-source check fails can you please check it?

Addressing the @whitesource-for-github-com WhiteSource Security Check Failing in PR #1679

@davidlago
Copy link

Wondering if we can retrigger whitesource check after that PR just merged... or perhaps just use admin to override-merge

@peternied peternied merged commit 714067e into opensearch-project:main Mar 15, 2022
@peternied peternied deleted the downgrade-gradle branch March 15, 2022 18:15
@peternied
Copy link
Member Author

@davidlago I just merged, when I attempted to re-run the tests it didn't retry the merge from main :/ If I was a more patient person I think rebasing + force push would be the way to go.

wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023
* Downgrade gradle version

BWC tooling is built with gradle 6, which has breaking changes that are
not compatiable with gradle 7.  In order to support BWC tests we need to
align with the OpenSearch's gradle version for the 1.3 release.

See Also:
* Gradle 7 PR in OpenSearch opensearch-project/OpenSearch#1622
* Distribution build bugs encountered by plugins opensearch-project/opensearch-build#1247
* Revert of Gradle 7 PR in OpenSearch  opensearch-project/OpenSearch#1657

Signed-off-by: Peter Nied <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default CI Java Version to Java 11, run tests on 8, 14 and 17
5 participants