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

Fixing issues caused by 3.0 version bumping #696

Merged
merged 9 commits into from
Oct 21, 2022
Merged

Fixing issues caused by 3.0 version bumping #696

merged 9 commits into from
Oct 21, 2022

Conversation

jackiehanyang
Copy link
Collaborator

@jackiehanyang jackiehanyang commented Oct 19, 2022

Description

Issues Resolved

opensearch-project/opensearch-plugins#142

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.

@opensearch-trigger-bot opensearch-trigger-bot bot added backport 2.x infra Changes to infrastructure, testing, CI/CD, pipelines, etc. labels Oct 19, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2022

Codecov Report

Merging #696 (c2ee2bf) into main (fd71844) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #696      +/-   ##
============================================
+ Coverage     79.03%   79.05%   +0.01%     
  Complexity     4241     4241              
============================================
  Files           301      301              
  Lines         17871    17872       +1     
  Branches       1897     1897              
============================================
+ Hits          14125    14129       +4     
+ Misses         2841     2838       -3     
  Partials        905      905              
Flag Coverage Δ
plugin 79.05% <ø> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...ensearch/ad/transport/ADCancelTaskNodeRequest.java 100.00% <ø> (ø)
...rg/opensearch/ad/transport/ADStatsNodeRequest.java 100.00% <ø> (ø)
...nsearch/ad/transport/ADTaskProfileNodeRequest.java 100.00% <ø> (ø)
...a/org/opensearch/ad/transport/CronNodeRequest.java 100.00% <ø> (ø)
...pensearch/ad/transport/DeleteModelNodeRequest.java 90.90% <ø> (ø)
...rg/opensearch/ad/transport/ProfileNodeRequest.java 100.00% <ø> (ø)
...ava/org/opensearch/ad/model/TimeConfiguration.java 100.00% <0.00%> (ø)
...java/org/opensearch/ad/task/ADBatchTaskRunner.java 82.06% <0.00%> (+0.30%) ⬆️
...ain/java/org/opensearch/ad/model/ModelProfile.java 72.72% <0.00%> (+1.81%) ⬆️

@jackiehanyang jackiehanyang marked this pull request as ready for review October 21, 2022 06:52
@jackiehanyang jackiehanyang requested a review from a team October 21, 2022 06:52
@@ -136,7 +136,7 @@ configurations.all {
force "joda-time:joda-time:${versions.joda}"
force "com.fasterxml.jackson.core:jackson-core:2.13.4"
force "commons-logging:commons-logging:${versions.commonslogging}"
force "org.apache.httpcomponents:httpcore:${versions.httpcore}"
force "org.apache.httpcomponents:httpcore5:${versions.httpcore5}"
Copy link
Member

Choose a reason for hiding this comment

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

Could we possible just remove this force? and just inherit it as is from opensearch core?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do you know what's the initial reason of adding this force instead of inheriting it from opensearch core?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, seems to be with our initial set up of AD. Usually from my understanding its used if we need a specific version of a dependency that doesn't align with core so we force it to be something else. I believe though since we aren't in need of anything specific we should be able to remove this line and build/testing should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

Before doing that I would confirm with @ylwu-amzn or @kaituo on the reasoning. When that's figured out, if it's still needed, we should add a clarifying comment above.

build.gradle Outdated Show resolved Hide resolved
@ohltyler
Copy link
Member

Can you link the related changes in core here for more context? For example, not sure of the reasoning of the new apache dependencies

@jackiehanyang
Copy link
Collaborator Author

Can you link the related changes in core here for more context? For example, not sure of the reasoning of the new apache dependencies

Updated the description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Changes to infrastructure, testing, CI/CD, pipelines, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants