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.

Sign up for GitHub

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

Jump to bottom

Add alpha1 qualifier and JDK 17 for backend #607

Merged
merged 10 commits into from
Mar 30, 2022
Merged

Add alpha1 qualifier and JDK 17 for backend #607

merged 10 commits into from
Mar 30, 2022

Conversation

joshuali925
Copy link
Member

Description

  • add alpha1 qualifier
  • add JDK 17
  • remove JDK 8 docs

Issues Resolved

[List any issues this PR will resolve]

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.

@joshuali925
Remove ref to jdk 8
5af2afa 
Signed-off-by: Joshua Li <joshuali925@gmail.com>
@joshuali925
Add alpha1 qualifier and JDK 17
703170c 
Signed-off-by: Joshua Li <joshuali925@gmail.com>
@joshuali925
Fix creating index with yml mappings
232ee72 
Signed-off-by: Joshua Li <joshuali925@gmail.com>
@joshuali925
Fix JDK 17 build issues
aa61e09 
Signed-off-by: Joshua Li <joshuali925@gmail.com>
@joshuali925
Remove unused variables
70a0a6b 
Signed-off-by: Joshua Li <joshuali925@gmail.com>
eugenesk24
eugenesk24 previously approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@eugenesk24 eugenesk24 left a comment

Choose a reason for hiding this comment

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

Yay looks good!

@joshuali925
Update opensearch_version
3af4921 
Signed-off-by: Joshua Li <joshuali925@gmail.com>
@joshuali925
Remove env vars in CI
d383ec5 
Signed-off-by: Joshua Li <joshuali925@gmail.com>
@joshuali925 joshuali925 marked this pull request as ready for review March 29, 2022 23:09
@joshuali925 joshuali925 requested a review from a team as a code owner March 29, 2022 23:09
@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2022 •
edited
Loading

Codecov Report

Merging #607 (74eff3a) into main (d3eb604) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main     #607   +/-   ##
=========================================
  Coverage     54.54%   54.54%           
  Complexity      283      283           
=========================================
  Files           239      239           
  Lines          7795     7795           
  Branches       1605     1605           
=========================================
  Hits           4252     4252           
  Misses         3371     3371           
  Partials        172      172
Flag Coverage Δ
dashboards-observability 46.12% <ø> (ø)
opensearch-observability 76.60% <100.00%> (ø)

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

Impacted Files Coverage Δ
...ensearch/observability/index/ObservabilityIndex.kt 57.83% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3eb604...74eff3a. Read the comment docs.

@joshuali925
Regex fix for multi digit versions
74eff3a 
Signed-off-by: Joshua Li <joshuali925@gmail.com>
davidcui1225
davidcui1225 approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@davidcui1225 davidcui1225 left a comment

Choose a reason for hiding this comment

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

LGTM- how come so many snapshots are being updated in the PR as well?

@joshuali925
Copy link
Member Author

LGTM- how come so many snapshots are being updated in the PR as well?

Dashboards core updated EUI in main branch which changed some components rendering, they don't have a 2.0 branch yet

eugenesk24
eugenesk24 reviewed Mar 30, 2022
View reviewed changes
opensearch-observability/build.gradle
if (System.getProperty("tests.clustername") != null) {
exclude 'org/opensearch/observability/ObservabilityPluginIT.class'
}
// if (System.getProperty("tests.clustername") != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to keep this commented out section?

Copy link
Member Author

Choose a reason for hiding this comment

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

i kept it because these are two issues: it fails when running against a remote cluster (same issue in index-management), and it fails under JDK 17 only. This is likely coming from upstream OpenSearchIntegTestCase class, keeping these lines in case it got fixed or we want to add more tests under it

eugenesk24
eugenesk24 approved these changes Mar 30, 2022
View reviewed changes
Copy link
Contributor

@eugenesk24 eugenesk24 left a comment

Choose a reason for hiding this comment

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

LG!

@joshuali925 joshuali925 merged commit 5f2294c into opensearch-project:main Mar 30, 2022
This was referenced Apr 7, 2022
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidcui1225 davidcui1225 davidcui1225 approved these changes

@eugenesk24 eugenesk24 eugenesk24 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joshuali925 @codecov-commenter @davidcui1225 @eugenesk24