-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Alerting] Add scoped cluster client to alerts and actions services #80794
[Alerting] Add scoped cluster client to alerts and actions services #80794
Conversation
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
LGTM, simple enough :-)
It would be good to have some tests though - I think maybe we can just change one of our existing functional tests to use the new client - would be good enough for me. Eg, one like this:
kibana/x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/server/alert_types.ts
Lines 79 to 89 in 8b6b1aa
await services.callCluster('index', { | |
index: params.index, | |
refresh: 'wait_for', | |
body: { | |
state, | |
params, | |
reference: params.reference, | |
source: 'alert:test.always-firing', | |
alertInfo, | |
}, | |
}); |
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.
Changes LGTM. Agreed, it could use a test, happy to help
51ffcf4
to
9e284d0
Compare
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
LGTM once a test is added, thanks for doing this! That said, please remove the resolves above as this doesn't actually resolve the issue- once the legacy client is removed, then we can consider #50247 resolved. :) Thanks ❤️ |
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.
LGTM, thanks for updating that function test to use the new client!
…lastic#80794) * Add scoped cluster client to alerts and actions services. * Modify functional test to use new ES client.
* master: (64 commits) Rename Security Solution Bug Template (elastic#81187) Update links (elastic#81125) Specify format for date range query (elastic#81025) [Alerting] Improve toast when alert is created (elastic#80327) [UX] Add empty states (elastic#80904) Add TS config for kibana_legacy (elastic#80992) [Telemetry] Add method to enable endpoint security data usage example (elastic#80940) [Alerting] Add scoped cluster client to alerts and actions services (elastic#80794) Fix reactRouterNavigate when used with a string (elastic#80520) [Security Solution] [Detections] Read privileges for dependencies (elastic#80852) [ML] Fixing exclude frequent in advanced wizard (elastic#81121) Fix security solution template label (elastic#80976) [DOCS] Update index management docs (elastic#80893) [APM] Error rate on service list page is not in sync with the value at the transaction page (elastic#80814) skip flaky suite (elastic#81072) [Task Manager] Cleans up legacy plugin structure (elastic#80381) Support unsigned_long fields (elastic#81115) [Form lib] Export internal state instead of raw state (elastic#80842) [Lens] Add toast notification when visualization is saved (elastic#80788) Index pattern edit field formatter API (elastic#78352) ...
Summary
As part of #80116 we discovered that one of the alerts required usage of
IndexPatternsFetcher
. Presently, the alert/action services do not provide a scoped client for this purpose. This is already on the alerting team's roadmap, so it will unblock the aforementioned patch and resolve the roadmap issue.Checklist
Delete any items that are not applicable to this PR.
For maintainers