-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Actions] adds a Test Connector tab in the Connectors list #77365
Conversation
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
* master: (65 commits) [Security Solution][Resolver] Analyzed event styling (elastic#77115) filter invalid SOs from the searc hresults in Task Manager (elastic#76891) [RUM Dashboard] Visitors by region map (elastic#77135) [Security Solution][Endpoint][Admin] Task/endpoint list actions (elastic#76555) [Ingest pipelines] Forms for processors T-U (elastic#76710) updating datatable type (elastic#77320) [ML] Fix custom URLs processing for security app (elastic#76957) [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747) [ML] Transforms: API schemas and integration tests (elastic#75164) [Mappings editor] Add support for wildcard field type (elastic#76574) [Ingest Manager] Fix flyout instruction selection (elastic#77071) [Telemetry Tools] update lodash to 4.17 (elastic#77317) [APM] Service inventory redesign (elastic#76744) Hide management sections based on cluster/index privileges (elastic#67791) [Snapshot Restore] Disable steps when form is invalid (elastic#76540) [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824) Update apm.ts (elastic#77310) [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164) [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986) [APM] API Snapshot Testing (elastic#77229) ...
@gchaps I would love your input on the copy here. The context is that we're offering the user am opportunity to test their connector by supplying the parameters for an action which will be executed using the connector. Copy I've added:
Any feedback on these labels? Happy to change them, they were all just off the cuff when added :) |
@gmmorris , some thoughts on the layout
|
Done :) |
1) Steps:
2) Waiting for input: When you run the action, the results will show up here. Note: Make it a single line like the warning message 3) Successful execution Action was successful 4) Failed execution Action failed to run The following error was found: Details: 5) Unsaved changes Save your changes before testing the connector. Other
|
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; exciting to try this live with a Slack action and see it post! Looking forward to trying some email errors, to see what they look like.
|
||
const err = find(result.items, 'index.error.reason'); | ||
if (err) { | ||
throw new Error( |
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.
Is there a reason we're throw
ing here instead of passing back a { status: 'error', ... }
object? I'm sure it will be handled fine, but I guess I tend to want to have action executors try to catch "common" errors and return the error status object - some day we may want to reserve catching errors in the caller as some kind of "misbehaving action executor" or such ...
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.
BTW, curious how you found this - seems like a general design bug with this action (not checking the bulk results for individual errors), don't remember seeing an issue for it tho.
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.
BTW, curious how you found this - seems like a general design bug with this action (not checking the bulk results for individual errors), don't remember seeing an issue for it tho.
Haha because it didn't fail! We swallowed the error and told the user it passed...
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.
Is there a reason we're
throw
ing here instead of passing back a{ status: 'error', ... }
object? I'm sure it will be handled fine, but I guess I tend to want to have action executors try to catch "common" errors and return the error status object - some day we may want to reserve catching errors in the caller as some kind of "misbehaving action executor" or such ...
Yeah, I considered that and decided to only have one error
handling block, but happy to change that 👍
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.
Code LGTM, but UX with required saving before have an option to test the params feels not friendly to me. Maybe I'm missing something, but why we just don't expose an additional server execute
endpoint which can take an action type, config, secrets (for an action create) and test params? For edit version we can execute endpoint without the secrets sending if it was not changed by the user and then just get it by the action id. With this kind of API we will have a solid experience of testing the action connector for create and edit pages. cc @mikecote
We decided to postpone that to a follow up issue as it would require a lot of refactoring in the action execution code (it's heavily reliant on the fact that Connectors are a SO at that point). Addressing this as part of this issue would mean a far bigger PR and the feeling was that the UX isn't bad, just not ideal. |
Thanks @gchaps
These are provided by the action itself and changes depending on the connector. Should we change this label there too? |
I don't think we want to let a user change the params without providing a secret, that would mean you could use some one else's secret to fire actions. I think that's a security hole. |
* master: (76 commits) Fixing service maps API test (elastic#77586) [Grok] Fix missing error message in error toasts (elastic#77499) [Alerting] Exempt Alerts pre 7.10 from RBAC on their Action execution until updated (elastic#75563) [ML] fix type in apply_influencer_filters_action (elastic#77495) [Lens] create reusable component for filters and range aggregation (elastic#77453) fix eslint violations Collapse alert chart previews by default (elastic#77479) [ML] Fixing field caps wrapper endpoint (elastic#77546) showing service maps when filte by environment not defined (elastic#77483) [Lens] Settings panel redesign and separate settings per y axis (elastic#76373) log request body in new ES client (elastic#77150) use `navigateToUrl` to navigate to recent nav links (elastic#77446) Move core config service to `kbn/config` package (elastic#76874) [UBI] Copy license to /licenses folder (elastic#77563) Skip flaky Events Viewer Cypress test [Lens] Remove dynamic names in telemetry fields (elastic#76988) [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366) [Enterprise Search] Add flag to restrict width of layout (elastic#77539) [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967) [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090) ...
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.
Looks good! Thank you!
Let's leave this text as is. |
@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
@elasticmachine merge upstream |
…7365) Adds a tab in the _Edit Alert_ flyout which allows the user to _test_ their connector by executing it using an example action. The execution relies on the connector being updated, so is only enabled when there are no saved changes in the Connector form itself.
…78134) Adds a tab in the _Edit Alert_ flyout which allows the user to _test_ their connector by executing it using an example action. The execution relies on the connector being updated, so is only enabled when there are no saved changes in the Connector form itself.
* master: (45 commits) [CSM] Use stacked chart for page views (elastic#78042) [Enterprise Search] Fix various plugin states when app has error connecting to Enterprise Search (elastic#78091) Remove service map beta badge (elastic#78039) [Enterprise Search] Rename "telemetry" to "stats" (elastic#78124) [Alerting] optimize calculation of unmuted alert instances (elastic#78021) call .destroy on ace when react component unmounts (elastic#78132) [Ingest Manager] Fix agent action acknowledgement (elastic#78089) [Upgrade Assistant] Rename "telemetry" to "stats" (elastic#78127) [Security Solution] Refactor Hosts Kpi to use Search Strategy (elastic#77606) Bump backport to 5.6.0 (elastic#78097) [Actions] adds a Test Connector tab in the Connectors list (elastic#77365) [Uptime] Improve ping chart axis (elastic#77992) [TSVB] Fields dropdowns are not populated if one of the indices is missing (elastic#77363) [UiActions] Remove duplicate apply filter action (elastic#77485) [APM] Use transaction metrics for transaction error rate (elastic#78009) [ES-ARCHIVER] Fix bug when query flag is empty (elastic#77983) Edit UI text strings in Integrations and Fleet tabs (elastic#75837) [baseline capture] switch to large workers (elastic#78109) [SECURITY_SOLUTION] list UI is backwards compatible (elastic#77956) [Mappings editor] Add support for point field type (elastic#77543) ...
💔 Build Failed
Failed CI StepsTest FailuresX-Pack Endpoint Functional Tests.x-pack/test/security_solution_endpoint/apps/endpoint/endpoint_list·ts.endpoint endpoint list when initially navigating to page finds data after load and pollingStandard Out
Stack Trace
X-Pack Endpoint Functional Tests.x-pack/test/security_solution_endpoint/apps/endpoint/endpoint_list·ts.endpoint endpoint list when initially navigating to page finds data after load and pollingStandard Out
Stack Trace
Metrics [docs]@kbn/optimizer bundle module count
async chunks size
distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
Summary
closes #75467
Adds a tab in the Edit Alert flyout which allows the user to test their connector by executing it using an example action.
The execution relies on the connector being updates, so is only enabled when there are no saved changes in the Connector form itself.
A followup issue has been created to add support for testing unsaved Connectors which will be usable prior to creating/modifying a connector in the system.
Designs
"Save & Test" button on creation
Edit mode
Waiting for test execution
There are unsaved changes
Test has ran and was successful
Test has ran and failed
Checklist
Delete any items that are not applicable to this PR.
For maintainers