[Actions] makes savedObjectId field optional #79186

gmmorris · 2020-10-01T18:12:55Z

Summary

This PR makes the savedObjectId parameter optional in the Jira, ServiceNow and IBM Resilient Connectors.
This allows them to execute without this field outside of Alerts, as it is currently populated using the alertId which isn't available in other places.
Additionally this adds an optional field in the Params Components for all three of the connectors, which allows users to provide a value for the savedObjectId field if the so wish.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-10-01T18:13:56Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

gmmorris · 2020-10-01T18:18:37Z

Hi @gchaps ,
Any advice on the title of the field?
It's currently Referenced Saved Object Id, which is technically correct, but not very informative.
Broadly the idea is to provide these services with an ID of a Saved Object in Kibana which is referenced by the API call to these services.

It isn't really in use yet, but longer term it would be used to link back to the Alert (or other Saved Object) which fired the Action.

In the context of Alerts we don't show this label, as it is automatically populated, so this field will only appear in situations where we don't know what Saved Object might be referenced here.

YulNaumenko

LGTM

gchaps · 2020-10-01T22:15:58Z

How about using a simple label:

Object ID

And something along these lines in a tooltip:

The API uses this ID to call the service.

gmmorris · 2020-10-02T11:17:52Z

How about using a simple label:

Object ID

And something along these lines in a tooltip:

The API uses this ID to call the service.

Will do, thanks :)

* master: (128 commits) add core-js production dependency (elastic#79395) Add support for sharing saved objects to all spaces (elastic#76132) [Alerting UI] Display a banner to users when some alerts have failures, added alert statuses column and filters (elastic#79038) load js-yaml lazily (elastic#79092) skip flaky suite (elastic#77278) Fix agentPolicyUpdateEventHandler() to use app context soClient for creation of actions (elastic#79341) [Security Solution] Untitled Timeline created when first action is to add note (elastic#78988) [Security Solutions][Detection Engine] Updates the edit rules page to:wq! only have what is selected for editing (elastic#79233) Cleanup yarn.lock from duplicates (elastic#66617) [kbn/optimizer] implement more efficient auto transpilation for node (elastic#79052) [Ingest Manager] Rename Fleet setup and requirement, Fleet => Central… (elastic#79291) [core/server/plugins] don't run discovery in dev server parent process (take 2) (elastic#79358) [babel/register] remove from build (take 2) (elastic#79379) [Security Solution] Changes rules table tag display (elastic#77102) define integrationTestRoot in config file and use to define screensho… (elastic#79247) Revert "[babel/register] remove from build (elastic#79176)" skip flaky suite (elastic#75241) [Uptime] Synthetics UI (elastic#77960) [Security Solution] [Detections] Only display actions options if user has "read" privileges (elastic#78812) [babel/register] remove from build (elastic#79176) ...

ymao1

Looks good!

pmuellr

LGTM, but it seems like the nullable on resilient is missing, and there is some extraneous code here (re: action execution results)

pmuellr · 2020-10-05T12:12:53Z

x-pack/plugins/actions/common/types.ts

+    unsafeResult &&
+    typeof unsafeResult === 'object' &&
+    unsafeResult?.actionId === 'string' &&
+    (unsafeResult?.status === 'ok' || unsafeResult?.status === 'error')


This seems fragile, in terms of if we ever change the status field to have values other than ok | error. I think doing a typeof ... === 'string' is probably fine here as well.

Oh, and just noticed the line above is probably intended to be the following (looks like it's missing typeof)

typeof unsafeResult?.actionId === 'string' &&

Yeah, it's definitely fragile - but I don't think there's another way around this as we do this in a catch and Typescript has no type checking on error handling. One of my main gripes with error throwing.

Do you have any advice for another way to do this?

It seems like the lines

unsafeResult?.actionId === 'string' && (unsafeResult?.status === 'ok' || unsafeResult?.status === 'error')

should be

typeof unsafeResult?.actionId === 'string' && typeof unsafeResult?.status === 'string'

pmuellr · 2020-10-05T12:15:13Z

x-pack/plugins/actions/server/builtin_action_types/servicenow/schema.ts

@@ -34,7 +34,7 @@ export const ExecutorSubActionSchema = schema.oneOf([
 ]);

 export const ExecutorSubActionPushParamsSchema = schema.object({
-  savedObjectId: schema.string(),
+  savedObjectId: schema.nullable(schema.string()),


I'm curious if this includes the SO type and spaceId. We can of course assume the spaceId for now. Is the type encoded in the string, or assumed to be alert? Could use a comment describing it.

I'm not touching the behaviour in this PR, just focusing on decoupling it from Alerting so that it's technically possible to execute.
It is in theory assuming this is an Alert, but apparently it isn't actually being used properly yet, so we decided to keep it because we already have this being sent to these services, but once we try to actually use these to generate URLs we'll definitely need to add the type.

LOL, github seems to have the diff confused here 🤔

pmuellr · 2020-10-05T12:24:42Z

...ggers_actions_ui/public/application/sections/action_connector_form/connector_edit_flyout.tsx

@@ -32,7 +32,10 @@ import { updateActionConnector, executeAction } from '../../lib/action_connector
 import { hasSaveActionsCapability } from '../../lib/capabilities';
 import { useActionsConnectorsContext } from '../../context/actions_connectors_context';
 import { PLUGIN } from '../../constants/plugin';
-import { ActionTypeExecutorResult } from '../../../../../actions/common';
+import {


Do the changes to execute, including the following files, belong in this PR? Seems like they should be in a separate one.

x-pack/plugins/actions/common/types.ts

x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api.ts,

They belong here in that without them it becomes impossible to correctly display the error in this PR.
It is addressing a broader problem that I identified while working on this. 🤷

Gotcha, thx!

x-pack/plugins/actions/server/builtin_action_types/jira/schema.ts

pmuellr

with the latest commits, LGTM

pmuellr · 2020-10-05T14:57:13Z

...ggers_actions_ui/public/application/sections/action_connector_form/connector_edit_flyout.tsx

@@ -32,7 +32,10 @@ import { updateActionConnector, executeAction } from '../../lib/action_connector
 import { hasSaveActionsCapability } from '../../lib/capabilities';
 import { useActionsConnectorsContext } from '../../context/actions_connectors_context';
 import { PLUGIN } from '../../constants/plugin';
-import { ActionTypeExecutorResult } from '../../../../../actions/common';
+import {


Gotcha, thx!

gchaps · 2020-10-05T15:18:56Z

@gmmorris Maybe this tweak to the tooltip:

JIRA will associate this action with the ID of a Kibana saved object.

gmmorris · 2020-10-05T15:42:01Z

JIRA will associate this action with the ID of a Kibana saved object.

Very last minute change is in 👍 😄

Thanks @gchaps

kibanamachine · 2020-10-05T17:14:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5253b25

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`triggers_actions_ui`	274	275	+1

async chunks size

id	before	after	diff
`securitySolution`	10.3MB	10.3MB	-4.0B
`triggers_actions_ui`	1.5MB	1.5MB	-19.8KB
total			-19.8KB

distributable file count

id	before	after	diff
`default`	47084	47087	+3

page load bundle size

id	before	after	diff
`securitySolution`	587.1KB	587.2KB	+46.0B
`triggers_actions_ui`	148.0KB	148.1KB	+87.0B
total			+133.0B

History

💚 Build #79620 succeeded 32964a7
💔 Build #79489 failed d99fe4b
💔 Build #78877 failed e62c1e6

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

This PR makes the `savedObjectId` parameter optional in the Jira, ServiceNow and IBM Resilient Connectors. This allows them to execute without this field outside of Alerts, as it is currently populated using the `alertId` which isn't available in other places. Additionally this adds an optional field in the `Params` Components for all three of the connectors, which allows users to provide a value for the `savedObjectId` field if the so wish.

gmmorris added 2 commits October 1, 2020 19:06

make savedObjectId field optional

9f2d66c

corrected i18n ids

e62c1e6

gmmorris requested a review from a team as a code owner October 1, 2020 18:12

gmmorris added Feature:Actions release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.10.0 v8.0.0 labels Oct 1, 2020

gmmorris requested a review from cnasikas October 1, 2020 18:14

YulNaumenko approved these changes Oct 1, 2020

View reviewed changes

gmmorris added 2 commits October 5, 2020 09:05

corrected copy and added tooltips

484c8d1

gmmorris changed the title ~~make savedObjectId field optional~~ [Actions] makes savedObjectId field optional Oct 5, 2020

ymao1 approved these changes Oct 5, 2020

View reviewed changes

pmuellr reviewed Oct 5, 2020

View reviewed changes

gmmorris added 4 commits October 5, 2020 14:11

fixed type check

7097aa9

corrected tests

9e07238

refactor isActionTypeExecutorResult

9453c18

refactor isActionTypeExecutorResult again

32964a7

pmuellr approved these changes Oct 5, 2020

View reviewed changes

copy update

5253b25

gmmorris merged commit 4fdf2f1 into elastic:master Oct 5, 2020

gmmorris mentioned this pull request Oct 5, 2020

[7.x] [Actions] makes savedObjectId field optional (#79186) #79515

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Actions] makes savedObjectId field optional #79186

[Actions] makes savedObjectId field optional #79186

gmmorris commented Oct 1, 2020 •

edited

Loading

elasticmachine commented Oct 1, 2020

gmmorris commented Oct 1, 2020

YulNaumenko left a comment

gchaps commented Oct 1, 2020

gmmorris commented Oct 2, 2020

ymao1 left a comment

pmuellr left a comment

pmuellr Oct 5, 2020

gmmorris Oct 5, 2020

pmuellr Oct 5, 2020

pmuellr Oct 5, 2020

gmmorris Oct 5, 2020

gmmorris Oct 5, 2020

pmuellr Oct 5, 2020

gmmorris Oct 5, 2020

pmuellr Oct 5, 2020

pmuellr left a comment

pmuellr Oct 5, 2020

gchaps commented Oct 5, 2020

gmmorris commented Oct 5, 2020

kibanamachine commented Oct 5, 2020

[Actions] makes savedObjectId field optional #79186

[Actions] makes savedObjectId field optional #79186

Conversation

gmmorris commented Oct 1, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Oct 1, 2020

gmmorris commented Oct 1, 2020

YulNaumenko left a comment

Choose a reason for hiding this comment

gchaps commented Oct 1, 2020

gmmorris commented Oct 2, 2020

ymao1 left a comment

Choose a reason for hiding this comment

pmuellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmuellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gchaps commented Oct 5, 2020

gmmorris commented Oct 5, 2020

kibanamachine commented Oct 5, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

distributable file count

page load bundle size

History

gmmorris commented Oct 1, 2020 •

edited

Loading