-
Notifications
You must be signed in to change notification settings - Fork 103
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
Register system index descriptors through SystemIndexPlugin.getSystemIndexDescriptors #1584
Register system index descriptors through SystemIndexPlugin.getSystemIndexDescriptors #1584
Conversation
…IndexDescriptors Signed-off-by: Craig Perkins <[email protected]>
@@ -117,6 +117,8 @@ class AlertIndices( | |||
/** The in progress alert history index. */ | |||
const val ALERT_INDEX = ".opendistro-alerting-alerts" | |||
|
|||
const val ALERT_CONFIG_INDEX = ".opendistro-alerting-config" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this here because I see it in the security repo, but I'm not able to find references to it in this repo. Where is this index declared and how is it used in the alerting plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bowenlan-amzn! I updated the PR to refer to this constant.
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
@bowenlan-amzn Can this be merged? |
@cwperks I notice there's one test failure
can you fix this? probably a common problem, not sure if the refreshAllIndices method needs to be updated |
@bowenlan-amzn looking into it. I will get back shortly. |
I was getting a ktlint error. The test passes with this change reverted. |
FYI I see another error as well. Does this need to be addressed?
|
@bowenlan-amzn I opened up a PR in core to address this: opensearch-project/OpenSearch#14635 |
@cwperks commented on Jul 3, 2024, 7:33 AM PDT:
If this is not causing a test to fail, it should be fine for now. |
…IndexDescriptors (#1584)
Issue #, if available:
This PR registers the system indices in this plugin through the SystemIndexPlugin extension point in core. These indices will not be functionally different than they are today, its just a formal registration as a system index.
Related to: opensearch-project/security#4439
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.