BugFix: Notification integration issues #339
Conversation
thalurur
force-pushed
the
notif
branch
3 times, most recently
from
6ba8c33 to
a353c11
Compare
Codecov Report
@@ Coverage Diff @@
## main #339 +/- ##
============================================
- Coverage 75.11% 74.91% -0.20%
+ Complexity 2139 2137 -2
============================================
Files 262 262
Lines 12425 12434 +9
Branches 1966 1966
============================================
- Hits 9333 9315 -18
- Misses 2029 2052 +23
- Partials 1063 1067 +4
Continue to review full report at Codecov.
|
Signed-off-by: Ravi Thaluru <[email protected]>
Signed-off-by: Ravi Thaluru <[email protected]>
| @@ -174,6 +174,8 @@ dependencies { | |||
| implementation "org.opensearch:common-utils:${common_utils_version}" | |||
| implementation "com.github.seancfoley:ipaddress:5.3.3" | |||
| implementation "commons-codec:commons-codec:${versions.commonscodec}" | |||
| implementation "org.apache.httpcomponents:httpclient:4.5.13" | |||
There was a problem hiding this comment.
Curious why these are added along with this change?
There was a problem hiding this comment.
It seems these are needed for the sending notification to legacy custom_webhook endpoints, the libraries are no longer in class path - I am not entirely sure where this was removed. Since IM needs it bundling it as part of the zip.
The PR is meant to fix bugs with notification plugin integration and since this is one of the bugs fixed it - I added a line in overview for this. However, I think I missed the renaming of PR - I updated it now
| it | ||
| ) | ||
| client.threadPool().threadContext.stashContext().use { | ||
| // We need to set the user context information in the thread context for notification plugin to correctly resolve the user object |
There was a problem hiding this comment.
Are we setting the user object twice in the threadContext? Here and above the calling stack in the runner?
There was a problem hiding this comment.
I believe the stash context will remove all the set variables but yes, we are setting it twice but in different format and in a different location in thread context
It seems notification plugin has special requirement for internal transport action calls than rest of the cluster
thalurur
changed the title
Signed-off-by: Ravi Thaluru <[email protected]>
Signed-off-by: Ravi Thaluru [email protected]
Issue #, if available:
N/A
Description of changes:
At the moment when notification plugin is called from IM the thread context is not populated with user information as a result when filter by backend roles is enabled in notification - IM won't be able to successfully send the notification. Updating logic to populate the thread context correctly
Added all the required run time dependencies to successfully send notifications through custom webhook
CheckList:
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.