Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

odfe 1.8 version upgrade #115

Merged
merged 7 commits into from
May 19, 2020
Merged

Conversation

vamshin
Copy link
Member

@vamshin vamshin commented May 19, 2020

Issue #, if available:
#113

Description of changes:
ODFE 1.8 support with Elasticsearch oss version 7.7.0

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@vamshin vamshin changed the title Od 1.8 odfe 1.8 version upgrade May 19, 2020
@vamshin vamshin requested a review from jmazanec15 May 19, 2020 04:57
Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

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

Minor comments. Just to confirm, everything worked as expected on MultiNode as well?

@@ -8,7 +8,7 @@ jobs:
build:
strategy:
matrix:
java: [13]
java: [14]
Copy link
Member

Choose a reason for hiding this comment

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

Need to update in other 2 Github actions as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Done

violationRules {
rule {
limit {
minimum = 0.5
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth adding a comment about test coverage in the ReadMe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if coverage needs to be added to README. Seems ok to me.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, probably not necessary.

@vamshin
Copy link
Member Author

vamshin commented May 19, 2020

Minor comments. Just to confirm, everything worked as expected on MultiNode as well?

Done sanity with multi nodes. Also ran ./gradlew :integTest -PnumNodes=2. Tests passed

Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

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

LGTM!

violationRules {
rule {
limit {
minimum = 0.5
Copy link
Member

Choose a reason for hiding this comment

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

Yes, probably not necessary.

@vamshin vamshin merged commit 9b5e114 into opendistro-for-elasticsearch:master May 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants