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

Bumping anomaly-detection to 1.1.0.0 to use OpenSearch (main) 1.1.0 #175

Merged
merged 2 commits into from
Aug 16, 2021

Conversation

VachaShah
Copy link
Contributor

@VachaShah VachaShah commented Aug 13, 2021

Signed-off-by: Vacha [email protected]

Description

Bumping upanomaly-detection to build with OpenSearch main which is 1.1.0 and use 1.1.0.0 for job-scheduler and common-utils.
As with the branching and release strategy for the project opensearch-project/.github#13
Here is how it should look like:

Plugin/library main builds out of OpenSearch main.
Plugin/library 1.x builds out of OpenSearch 1.x.
Plugin/library 1.0 builds out of OpenSearch 1.0.
This allows us to fail fast and get ready for release.

As part of adding backwards compatibility test framework, we are developing tests as an example in Anomaly Detection which needs the bwc framework from OpenSearch main.
Ref: opensearch-project/OpenSearch#1002
Meta issue: opensearch-project/opensearch-build#90

Issues Resolved

#156

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.

kaituo
kaituo previously approved these changes Aug 13, 2021
@VachaShah VachaShah force-pushed the bump-to-1.1.0 branch 2 times, most recently from d787d51 to 3817101 Compare August 13, 2021 22:42
@@ -33,7 +33,7 @@ jobs:
with:
repository: 'opensearch-project/OpenSearch'
path: OpenSearch
ref: '1.0'
ref: 'main'
- name: Build OpenSearch
working-directory: ./OpenSearch
run: ./gradlew publishToMavenLocal -Dbuild.snapshot=false
Copy link
Member

Choose a reason for hiding this comment

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

We are consuming OpenSearch 1.1.0-SNAPSHOT but publishing OpenSearch 1.1.0 to maven.
Can you move it to publish the snapshot, this would unblock the builds in CI.

Suggested change
run: ./gradlew publishToMavenLocal -Dbuild.snapshot=false
run: ./gradlew publishToMavenLocal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed this, I have updated it. Thank you :)

@@ -274,7 +274,7 @@ String bwcFilePath = "src/test/resources/org/opensearch/ad/bwc/"
testClusters {
"${baseName}" {
testDistribution = "ARCHIVE"
versions = ["7.10.2","1.0.0"]
versions = ["7.10.2","1.1.0-SNAPSHOT"]
Copy link
Collaborator

@ylwu-amzn ylwu-amzn Aug 13, 2021

Choose a reason for hiding this comment

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

Should we also test upgrade with last version OS 1.0?
BTW, the CI workflow failed as "Could not find org.opensearch.gradle:build-tools:1.1.0."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ylwu-amzn, the corresponding bwc tests are added so that the core framework for bwc for plugins is ready to use and they are an example of how the bwc tests can be added in the plugins. Each plugin owners can now take up the tests and enhance them to include multiple versions and test required functionalities.

For the CI failure, I am working on fixing that since AD is consuming the snapshot build while job-scheduler and common-utils are not, I am making corresponding changes in the respective repos.

@codecov-commenter
Copy link

Codecov Report

Merging #175 (e977396) into main (fa081d4) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #175      +/-   ##
============================================
+ Coverage     67.64%   67.69%   +0.05%     
- Complexity     2885     2887       +2     
============================================
  Files           270      270              
  Lines         14104    14104              
  Branches       1418     1418              
============================================
+ Hits           9540     9548       +8     
+ Misses         3971     3967       -4     
+ Partials        593      589       -4     
Flag Coverage Δ
plugin 67.69% <100.00%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
src/main/java/org/opensearch/ad/util/Bwc.java 100.00% <100.00%> (+33.33%) ⬆️
...port/SearchAnomalyDetectorInfoTransportAction.java 55.55% <0.00%> (-8.89%) ⬇️
...opensearch/ad/indices/AnomalyDetectionIndices.java 50.93% <0.00%> (+0.31%) ⬆️
.../handler/IndexAnomalyDetectorJobActionHandler.java 28.57% <0.00%> (+0.71%) ⬆️
.../opensearch/ad/cluster/ADClusterEventListener.java 90.90% <0.00%> (+1.81%) ⬆️
...opensearch/ad/transport/AnomalyResultResponse.java 77.63% <0.00%> (+3.94%) ⬆️
.../java/org/opensearch/ad/AnomalyDetectorRunner.java 44.82% <0.00%> (+5.74%) ⬆️

@dblock
Copy link
Member

dblock commented Aug 14, 2021

This plus #176 is probably what we want to default the build to produce a snapshot version.

@dblock
Copy link
Member

dblock commented Aug 14, 2021

Anomaly-detection main branch is now 1.1 and we're not working on 2.0. I think this means it should depend on OpenSearch 1.x branch (right now OpenSearch/main is 2.0)?

@dblock
Copy link
Member

dblock commented Aug 16, 2021

I'll merge this to get unblocked for #176.

@dblock dblock merged commit c4ed5a7 into opensearch-project:main Aug 16, 2021
@VachaShah VachaShah deleted the bump-to-1.1.0 branch August 17, 2021 16:59
ohltyler pushed a commit to ohltyler/anomaly-detection-2 that referenced this pull request Sep 1, 2021
ohltyler pushed a commit to ohltyler/anomaly-detection-2 that referenced this pull request Sep 1, 2021
@ohltyler ohltyler added the infra Changes to infrastructure, testing, CI/CD, pipelines, etc. label Sep 2, 2021
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.

7 participants