-
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
[SIEM][CASE] Refactor Connectors - Jira Connector #63450
Conversation
Pinging @elastic/siem (Team:SIEM) |
0f22570
to
e049e11
Compare
f2f1134
to
3dcec08
Compare
x-pack/plugins/actions/server/builtin_action_types/connectors/schema.ts
Outdated
Show resolved
Hide resolved
90aff76
to
94814be
Compare
94814be
to
763c60e
Compare
x-pack/legacy/plugins/siem/public/containers/case/use_post_push_to_service.test.tsx
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/siem/public/lib/connectors/resilient/config.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/siem/public/lib/connectors/resilient/index.tsx
Outdated
Show resolved
Hide resolved
41388a8
to
ef46abf
Compare
d2207fe
to
3603141
Compare
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.
@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.
I looked through the code that touches actions, looks good, except for some comments on schema.maybe()
usage that should be changed.
Thanks for making the name changes I suggested in a previous review!
export const CommentSchema = schema.object({ | ||
commentId: schema.string(), | ||
comment: schema.string(), | ||
version: schema.maybe(schema.string()), |
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.
You should be using schema.nullable()
here instead of schema.maybe()
. Otherwise, if the field is set at some point, and then you'd like to "null it out", there's no way to do that, as the updates we make to the SO are partial updates, so the effect would be that the field is unchanged.
There are a couple of other references to schema.maybe()
in here as well.
I'll note that it's unwieldy to use the TS types generated from schema.nullable()
to generate "writable" data structures in code, as TS will require the fields to be set to undefined
or null
- you can't simply just not set the field. But I believe that's true for schema.maybe()
as well, and in general, using the the TS types generated from schema to use with programmatically built data structures is ... can be a bit painful.
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.
You are right about nullable
. I do not know why we started with undefined
. The UI sent null for non-used values. Thanks a lot for catching this.
|
||
export const UserSchema = schema.object({ | ||
fullName: schema.oneOf([schema.nullable(schema.string()), schema.maybe(schema.string())]), | ||
username: schema.oneOf([schema.nullable(schema.string()), schema.maybe(schema.string())]), |
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 made a note about the use of schema.maybe()
in another comment, but this one seems ... more interesting. I think technically you can just use fullName: schema.nullable(schema.string())
, but am curious if you had to do it this way for some other reason. Even for cases like MappingActionType
above, I've instead sometimes just set the schema to a string, and validated the fixed set of literals in a custom validator, to produce a better error message for validation.
Probably worthwhile noting that the validation error messages produced from schema.oneOf()
are quite verbose, and can be a little confusing.
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.
Not a good reason for that either. Probably I did it to match fullname: string | undefined | null
. You are right, no need for that.
About schema.oneOf
you are right. It's quite verbose and difficult to catch. I tried to use the validate function but there was a bug (#64906). I would follow your advice and create a custom validator to produce better error messages for our schema in another PR.
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.
realized in my previous review I just "commented", should have "approved". But see #63450 (comment) regarding schema changes.
I saw your previous comments and made the appropriate changes. Thank you, I learned a lot! |
* upstream/master: (315 commits) [APM] Fix failing `ApmIndices` test (elastic#64965) [APM] Fix paths for ts optimization script (elastic#65012) Use HDR for percentiles (elastic#64758) [EPM] fix updates available filter (elastic#64957) [Uptime] Certificates page (elastic#64059) load lens app lazily (elastic#64769) [legacy/server/config] remove unnecessary deps for simple helper (elastic#64954) Fixed alert Edit flyout shows the error message when one of this actions has a preconfigured action type (elastic#64742) [data.search.aggs] Remove legacy aggs APIs. (elastic#64719) Fixed `AddAlert` flyout does not immediately update state to reflect new props (elastic#64927) [Discover] Show doc viewer action buttons on focus (elastic#64912) [EPM] restrict package install endpoint from installing/updating to old packages (elastic#64932) [Metrics UI] Add inventory metric threshold alerts (elastic#64292) [Canvas] Adds edit menu (elastic#64738) [Canvas] Adds refresh and autoplay options to view menu (elastic#64375) [Lens] Trigger a filter action on click in datatable visualization (elastic#63840) [SIEM][CASE] Refactor Connectors - Jira Connector (elastic#63450) [APM] Client new platform migration (elastic#64046) [Monitoring] NP Migration complete client cutover (elastic#62908) Ingest Node Pipelines UI (elastic#62321) ...
…or-part-mvp-2 * 'master' of github.com:elastic/kibana: (51 commits) [APM] Fix failing `ApmIndices` test (elastic#64965) [APM] Fix paths for ts optimization script (elastic#65012) Use HDR for percentiles (elastic#64758) [EPM] fix updates available filter (elastic#64957) [Uptime] Certificates page (elastic#64059) load lens app lazily (elastic#64769) [legacy/server/config] remove unnecessary deps for simple helper (elastic#64954) Fixed alert Edit flyout shows the error message when one of this actions has a preconfigured action type (elastic#64742) [data.search.aggs] Remove legacy aggs APIs. (elastic#64719) Fixed `AddAlert` flyout does not immediately update state to reflect new props (elastic#64927) [Discover] Show doc viewer action buttons on focus (elastic#64912) [EPM] restrict package install endpoint from installing/updating to old packages (elastic#64932) [Metrics UI] Add inventory metric threshold alerts (elastic#64292) [Canvas] Adds edit menu (elastic#64738) [Canvas] Adds refresh and autoplay options to view menu (elastic#64375) [Lens] Trigger a filter action on click in datatable visualization (elastic#63840) [SIEM][CASE] Refactor Connectors - Jira Connector (elastic#63450) [APM] Client new platform migration (elastic#64046) [Monitoring] NP Migration complete client cutover (elastic#62908) Ingest Node Pipelines UI (elastic#62321) ... # Conflicts: # x-pack/plugins/ingest_pipelines/common/types.ts # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx # x-pack/plugins/ingest_pipelines/public/shared_imports.ts
* master: (44 commits) onEvent prop for expression component (elastic#64995) [APM] Fix failing `ApmIndices` test (elastic#64965) [APM] Fix paths for ts optimization script (elastic#65012) Use HDR for percentiles (elastic#64758) [EPM] fix updates available filter (elastic#64957) [Uptime] Certificates page (elastic#64059) load lens app lazily (elastic#64769) [legacy/server/config] remove unnecessary deps for simple helper (elastic#64954) Fixed alert Edit flyout shows the error message when one of this actions has a preconfigured action type (elastic#64742) [data.search.aggs] Remove legacy aggs APIs. (elastic#64719) Fixed `AddAlert` flyout does not immediately update state to reflect new props (elastic#64927) [Discover] Show doc viewer action buttons on focus (elastic#64912) [EPM] restrict package install endpoint from installing/updating to old packages (elastic#64932) [Metrics UI] Add inventory metric threshold alerts (elastic#64292) [Canvas] Adds edit menu (elastic#64738) [Canvas] Adds refresh and autoplay options to view menu (elastic#64375) [Lens] Trigger a filter action on click in datatable visualization (elastic#63840) [SIEM][CASE] Refactor Connectors - Jira Connector (elastic#63450) [APM] Client new platform migration (elastic#64046) [Monitoring] NP Migration complete client cutover (elastic#62908) ...
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
Changes on this PR:
Dev Docs
The API changed to support executor actions. The supported action are:
pushToService
,handshake
, andgetIncident
. This PR implements only thepushToService
action.The following response fields have changed:
incidentId
changed toid
.number
changed totitle
.Create an incident:
Create an incident to ServiceNow. When the
incidentId
attribute is not inactionParams
the executor will create the incident.Endpoint:
api/action/<action_id>/_execute
Method:
POST
Payload:
Response
Update an incident:
Update an incident to ServiceNow. When the
incidentId
attribute is inactionParams
the executor will update the incident.Endpoint:
api/action/<action_id>/_execute
Method:
POST
Payload:
Response
Checklist
Delete any items that are not applicable to this PR.
For maintainers