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

Fix CI #1760

Merged
merged 3 commits into from
Jun 22, 2023
Merged

Fix CI #1760

merged 3 commits into from
Jun 22, 2023

Conversation

Yury-Fridlyand
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand commented Jun 21, 2023

Description

In opensearch-project/OpenSearch#8035 some classes were moved from core to common-utils without renaming or changing hierarchy. In ml-commons in opensearch-project/ml-commons#993 a new dependency was added to keep things up to date, but the same dependency should be added in SQL plugin as well.
This fix adds:

  1. common-utils dependency to :opensearch
  2. updates mockito version in plugin (:opensearch-sql-plugin) to avoid conflicts between 5.2.0 and 5.4.0 versions. 5.4.0 comes from upstream.
Execution failed for task ':opensearch-sql-plugin:compileTestJava'.
> Could not resolve all dependencies for configuration ':opensearch-sql-plugin:testCompileClasspath'.
   > Conflict(s) found for the following module(s):
       - org.mockito:mockito-core between versions 5.4.0 and 5.2.0
     Run with:
         --scan or
         :opensearch-sql-plugin:dependencyInsight --configuration testCompileClasspath --dependency org.mockito:mockito-core
     to get more insight on how to solve the conflict.

Issues Resolved

#1753

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • 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: Yury-Fridlyand <[email protected]>
@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #1760 (66b98fd) into main (bb020ac) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1760   +/-   ##
=========================================
  Coverage     97.29%   97.29%           
  Complexity     4408     4408           
=========================================
  Files           388      388           
  Lines         10944    10944           
  Branches        774      774           
=========================================
  Hits          10648    10648           
  Misses          289      289           
  Partials          7        7           
Flag Coverage Δ
sql-engine 97.29% <ø> (ø)

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

@@ -39,6 +39,7 @@ dependencies {
implementation group: 'org.json', name: 'json', version:'20230227'
compileOnly group: 'org.opensearch.client', name: 'opensearch-rest-high-level-client', version: "${opensearch_version}"
implementation group: 'org.opensearch', name:'opensearch-ml-client', version: "${opensearch_build}"
implementation group: 'org.opensearch', name: 'common-utils', version: "${opensearch_build}"
Copy link
Member

@vmmusings vmmusings Jun 21, 2023

Choose a reason for hiding this comment

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

Did this fix the NoClassDef error?
Or it automatically got fixed after the latest changes got pushed to sonatype snaphot repo. https://aws.oss.sonatype.org/content/repositories/snapshots/org/opensearch/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just restarted CI on main it failed: https://github.com/opensearch-project/sql/actions/runs/5320847765/jobs/9655483529. So fix is still required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Re-run the CI, only see the following errors. Does common-utils necessary?

Execution failed for task ':opensearch-sql-plugin:compileTestJava'.
AstExpressionBuilderTest > canBuildArithmeticExpressionPrecedence() PASSED
> Could not resolve all dependencies for configuration ':opensearch-sql-plugin:testCompileClasspath'.

   > Conflict(s) found for the following module(s):
AstExpressionBuilderTest > canBuildComparisonExpression() PASSED
       - org.mockito:mockito-core between versions 5.4.0 and 5.2.0
     Run with:
         --scan or
         :opensearch-sql-plugin:dependencyInsight --configuration testCompileClasspath --dependency org.mockito:mockito-core
     to get more insight on how to solve the conflict.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, perhaps snapshots was updated which makes this dependency unneeded. Reverted in 66b98fd.
Thank you for testing!

@vmmusings
Copy link
Member

common-utils

Nothing got moved to common-utils. By common-utilsare you referring to this https://github.com/opensearch-project/common-utils?
I am trying to understand the root cause of the CI issue and how did the current PR changes fixed.

acarbonetto
acarbonetto previously approved these changes Jun 21, 2023
Copy link
Collaborator

@acarbonetto acarbonetto left a comment

Choose a reason for hiding this comment

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

Adding more dependencies makes me not happy, but I suppose it's for a good cause (breaking up the monolithic core library).

forestmvey
forestmvey previously approved these changes Jun 21, 2023
GumpacG
GumpacG previously approved these changes Jun 21, 2023
vmmusings
vmmusings previously approved these changes Jun 21, 2023
@Yury-Fridlyand Yury-Fridlyand dismissed stale reviews from vmmusings and GumpacG via 66b98fd June 22, 2023 00:52
Copy link
Collaborator

@penghuo penghuo left a comment

Choose a reason for hiding this comment

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

Thanks for the change!

@Yury-Fridlyand Yury-Fridlyand merged commit f6e2a97 into opensearch-project:main Jun 22, 2023
@Yury-Fridlyand Yury-Fridlyand deleted the fix-ci branch June 22, 2023 16:35
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 22, 2023
* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <[email protected]>

---------

Signed-off-by: Yury-Fridlyand <[email protected]>
(cherry picked from commit f6e2a97)
acarbonetto pushed a commit that referenced this pull request Jun 22, 2023
* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <[email protected]>

---------

Signed-off-by: Yury-Fridlyand <[email protected]>
(cherry picked from commit f6e2a97)

Co-authored-by: Yury-Fridlyand <[email protected]>
matthewryanwells pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Jun 22, 2023
* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <[email protected]>

---------

Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Matthew Wells <[email protected]>
acarbonetto pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Jun 22, 2023
* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <[email protected]>

---------

Signed-off-by: Yury-Fridlyand <[email protected]>
MitchellGale pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Jun 22, 2023
* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <[email protected]>

---------

Signed-off-by: Yury-Fridlyand <[email protected]>
Yury-Fridlyand added a commit that referenced this pull request Jun 22, 2023
…s upon project failure in startOpenSearch (#1763)

* fixed buildgradle merge conflicts

Signed-off-by: Mitchell Gale <[email protected]>

* Fix CI (#1760)

* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <[email protected]>

---------

Signed-off-by: Yury-Fridlyand <[email protected]>

---------

Signed-off-by: Mitchell Gale <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
Yury-Fridlyand added a commit that referenced this pull request Jun 27, 2023
…xt (#1779)

* Fix CI (#1760)

* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <[email protected]>

---------

Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Matthew Wells <[email protected]>

* Fix CSV/RAW outputting wrong format (#279)

* Fixed bug where CSV/RAW outputs as JSON rather than plain text

Signed-off-by: Matthew Wells <[email protected]>

---------

Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Matthew Wells <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 27, 2023
…xt (#1779)

* Fix CI (#1760)

* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <[email protected]>

---------

Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Matthew Wells <[email protected]>

* Fix CSV/RAW outputting wrong format (#279)

* Fixed bug where CSV/RAW outputs as JSON rather than plain text

Signed-off-by: Matthew Wells <[email protected]>

---------

Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Matthew Wells <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
(cherry picked from commit 1ec696d)
Yury-Fridlyand pushed a commit that referenced this pull request Jun 28, 2023
* Fix CSV/RAW output header being application/json rather than plain/text (#1779)

* Fix CI (#1760)

* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <[email protected]>

---------

Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Matthew Wells <[email protected]>

* Fix CSV/RAW outputting wrong format (#279)

* Fixed bug where CSV/RAW outputs as JSON rather than plain text

Signed-off-by: Matthew Wells <[email protected]>

---------

Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Matthew Wells <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
(cherry picked from commit 1ec696d)

* updated tests

Signed-off-by: Matthew Wells <[email protected]>

* updated tests to return name

Signed-off-by: Matthew Wells <[email protected]>

* changed tests to return value

Signed-off-by: Matthew Wells <[email protected]>

* removed unneeded imports

Signed-off-by: Matthew Wells <[email protected]>

---------

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

Successfully merging this pull request may close these issues.

9 participants