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

Removal of NotificationEvent Request, Response and SearchResults #395

Merged
merged 9 commits into from
Apr 8, 2022
Merged

Removal of NotificationEvent Request, Response and SearchResults #395

merged 9 commits into from
Apr 8, 2022

Conversation

adityaj1107
Copy link
Contributor

@adityaj1107 adityaj1107 commented Apr 4, 2022

Description

This PR removes:

  • CRUD operations on Notifications Index
  • Data Models for NotificationEventDocInfo
  • Creation and Maintenance of Notifications Event Index.
  • Rest Handler for Notifications Event.
  • Data Models for NotificationEventDoc and NotificationEventDocInfo
  • IT for Querying Notification Event.
  • Events API Assertions from SendTestMessageRestHandler
  • Cleanup in SendMessageActionHelper
  • Tests for NotificationEventDocTests , NotificationEventIndexTests, PluginActionTests, ``

Issues Resolved

Removal of code as part of: #394

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.

@adityaj1107 adityaj1107 requested a review from a team April 4, 2022 18:06
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2022

Codecov Report

Merging #395 (ecd8ee8) into main (6a0e36b) will decrease coverage by 25.23%.
The diff coverage is 90.32%.

@@              Coverage Diff              @@
##               main     #395       +/-   ##
=============================================
- Coverage     86.40%   61.17%   -25.24%     
- Complexity        0       81       +81     
=============================================
  Files            51       73       +22     
  Lines          1479     2421      +942     
  Branches        352      264       -88     
=============================================
+ Hits           1278     1481      +203     
- Misses          198      763      +565     
- Partials          3      177      +174     
Flag Coverage Δ
dashboards-notifications ?
opensearch-notifications 61.17% <90.32%> (?)

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

Impacted Files Coverage Δ
...nsearch/notifications/core/NotificationCoreImpl.kt 100.00% <ø> (ø)
...earch/notifications/core/setting/PluginSettings.kt 74.75% <ø> (ø)
...search/notifications/model/NotificationEventDoc.kt 100.00% <ø> (ø)
...earch/notifications/action/GetChannelListAction.kt 81.81% <66.66%> (ø)
...arch/notifications/send/SendMessageActionHelper.kt 48.39% <66.66%> (ø)
...org/opensearch/notifications/NotificationPlugin.kt 97.50% <100.00%> (ø)
...ch/notifications/action/GetPluginFeaturesAction.kt 84.61% <100.00%> (ø)
...earch/notifications/index/ConfigIndexingActions.kt 54.80% <100.00%> (ø)
...rch/notifications/index/NotificationConfigIndex.kt 77.34% <100.00%> (ø)
...in/org/opensearch/notifications/metrics/Metrics.kt 96.85% <100.00%> (ø)
... and 125 more

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 dabedb0...ecd8ee8. Read the comment docs.

@adityaj1107
Copy link
Contributor Author

Dependency on this PR: opensearch-project/common-utils#153

@qreshi
Copy link
Contributor

qreshi commented Apr 6, 2022

By the way, did you get a chance to locally run this with the frontend as well and just did a sanity check there?

It looks like you're doing a follow-up on the SendMessage/SendTestMessage responses later so that doesn't have to work for now but just to check that everything else is fine would be good since the frontend GitHub Action is failing.

Also, if you do test the frontend, I'd recommend using commit 920cd5e45d28784ebd6406e332d2275efecd6026 of OpenSearch Dashboards for now. It looks like the commits after that started causing the Notifications Dashboards bootstrap to fail (at least up to commit a62dd96f11a5a99bbe73069ccf7cf66d26af67c9 which was the newest one when I tested it).

@qreshi
Copy link
Contributor

qreshi commented Apr 7, 2022

@aditjind Looks like there are compile errors after your recent commits.

@adityaj1107
Copy link
Contributor Author

@aditjind Looks like there are compile errors after your recent commits.

Build depends on this PR: opensearch-project/common-utils#156

@adityaj1107
Copy link
Contributor Author

adityaj1107 commented Apr 7, 2022

Build Passing Locally as this PR is needed: opensearch-project/common-utils#156

> Task :opensearch-notifications-core:test
Java HotSpot(TM) 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended
Java HotSpot(TM) 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended
Java HotSpot(TM) 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended
Java HotSpot(TM) 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended

> Task :jacocoReport
[ant:jacocoReport] Classes in bundle 'opensearch-notifications' do not match with execution data. For report generation the same class files must be used as at runtime.
[ant:jacocoReport] Execution data for class org/opensearch/notifications/spi/model/destination/BaseDestination does not match.
[ant:jacocoReport] Execution data for class org/opensearch/notifications/spi/model/destination/SlackDestination does not match.
[ant:jacocoReport] Execution data for class org/opensearch/notifications/spi/model/destination/CustomWebhookDestination does not match.
[ant:jacocoReport] Execution data for class org/opensearch/notifications/spi/model/destination/DestinationType does not match.
[ant:jacocoReport] Execution data for class org/opensearch/notifications/spi/model/destination/ChimeDestination does not match.
[ant:jacocoReport] Execution data for class org/opensearch/notifications/spi/model/DestinationMessageResponse does not match.
[ant:jacocoReport] Execution data for class org/opensearch/notifications/spi/model/MessageContent does not match.
[ant:jacocoReport] Execution data for class org/opensearch/notifications/spi/model/destination/SnsDestination does not match.
[ant:jacocoReport] Execution data for class org/opensearch/notifications/spi/model/SecureDestinationSettings does not match.
[ant:jacocoReport] Execution data for class org/opensearch/notifications/spi/model/destination/WebhookDestination does not match.
[ant:jacocoReport] Execution data for class org/opensearch/notifications/spi/model/destination/SmtpDestination does not match.

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.3/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 5m 41s
55 actionable tasks: 53 executed, 2 up-to-date

Signed-off-by: Jindal <[email protected]>
@adityaj1107 adityaj1107 requested a review from qreshi April 7, 2022 23:57
@qreshi qreshi merged commit 1b8421c into opensearch-project:main Apr 8, 2022
@adityaj1107 adityaj1107 deleted the events-doc-removal-2-main branch April 17, 2022 23:34
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.

5 participants