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

Move extension settings reading into the BaseExtension #205

Merged
merged 6 commits into from
Nov 4, 2022

Conversation

mloufra
Copy link
Contributor

@mloufra mloufra commented Oct 28, 2022

Signed-off-by: mloufra [email protected]

Description

Move the extensionSettings field and the initialization in the constructor and all the exception handling in HelloWorldExtension to BaseExtension.

Issues Resolved

Fixes #182

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.

@mloufra mloufra requested a review from a team October 28, 2022 04:33
@codecov-commenter
Copy link

Codecov Report

Merging #205 (567793a) into main (35f8d2b) will not change coverage.
The diff coverage is 55.55%.

@@            Coverage Diff            @@
##               main     #205   +/-   ##
=========================================
  Coverage     65.86%   65.86%           
  Complexity      103      103           
=========================================
  Files            25       25           
  Lines           501      501           
  Branches         17       17           
=========================================
  Hits            330      330           
  Misses          159      159           
  Partials         12       12           
Impacted Files Coverage Δ
...rch/sdk/sample/helloworld/HelloWorldExtension.java 66.66% <ø> (+9.52%) ⬆️
...rc/main/java/org/opensearch/sdk/BaseExtension.java 71.42% <55.55%> (-28.58%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

src/main/java/org/opensearch/sdk/BaseExtension.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/sdk/BaseExtension.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/sdk/BaseExtension.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/sdk/BaseExtension.java Outdated Show resolved Hide resolved
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

Looking good! A few more tweaks needed!

@mloufra
Copy link
Contributor Author

mloufra commented Oct 31, 2022

Looking good! A few more tweaks needed!

Thank you for you comment @dbwiddis

@mloufra mloufra force-pushed the moveextensionsetting branch from 2c4311a to b71a1b5 Compare November 2, 2022 18:16
owaiskazi19
owaiskazi19 previously approved these changes Nov 2, 2022
dbwiddis
dbwiddis previously approved these changes Nov 2, 2022
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM! Great job, I think this will really simplify the experience for users!

@owaiskazi19 FYI, once this is merged you may want to simplify the AD Extension constructor as well.

@owaiskazi19
Copy link
Member

@dbwiddis can you resolve the comments if they look good to you? Also, let's hold merging this until we fix the build error

@dbwiddis
Copy link
Member

dbwiddis commented Nov 2, 2022

@dbwiddis can you resolve the comments if they look good to you? Also, let's hold merging this until we fix the build error

Done.

@dbwiddis dbwiddis dismissed stale reviews from owaiskazi19 and themself via 51a0e23 November 4, 2022 19:15
@dbwiddis dbwiddis force-pushed the moveextensionsetting branch from b71a1b5 to 51a0e23 Compare November 4, 2022 19:15
@owaiskazi19
Copy link
Member

@dbwiddis I'll let you merge this one based on the sequence to avoid merge conflicts

@dbwiddis dbwiddis merged commit d07e341 into opensearch-project:main Nov 4, 2022
@mloufra mloufra deleted the moveextensionsetting branch November 4, 2022 20:06
kokibas pushed a commit to kokibas/opensearch-sdk-java that referenced this pull request Mar 17, 2023
…oject#205)

* move extensionSetting to BaseExtension

Signed-off-by: mloufra <[email protected]>

* address comment

Signed-off-by: mloufra <[email protected]>

* address comment

Signed-off-by: mloufra <[email protected]>

* fix spotless error

Signed-off-by: mloufra <[email protected]>

* fix setting initialization error

Signed-off-by: mloufra <[email protected]>

* add new constructor in BaseExtensions

Signed-off-by: mloufra <[email protected]>

Signed-off-by: mloufra <[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.

[FEATURE] Move extension settings reading into the BaseExtension
4 participants