Skip to content
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

Make the actions plugin support generics #71439

Merged
merged 18 commits into from
Aug 4, 2020

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Jul 13, 2020

Solves the actions portion of #64147

In this PR, I'm adding support for generics in the actions plugin to the following types:

  • ActionTypeExecutorOptions<Config, Secrets, Params>
  • ActionResult<Config> (optional type)
  • PreConfiguredAction<Config, Secrets> (optional types)
  • ActionTypeExecutorResult<Data> (optional type)
  • ExecutorType<Config, Secrets, Params>
  • ActionType<Config, Secrets, Params> (optional types)

Example

export type ServerLogActionTypeParams = { message: string };

const serverLogActionType: ActionType<{}, {}, ServerLogActionTypeParams> = {
  ...
  async executor(execOptions) {
    // Below is considered a string with the type pulled from ServerLogActionTypeParams['message']
    const { message } = execOptions.params;
  }
}

@mikecote mikecote added Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Jul 13, 2020
@mikecote mikecote self-assigned this Jul 13, 2020
@gmmorris gmmorris self-requested a review July 21, 2020 09:07
@apmmachine
Copy link
Contributor

apmmachine commented Jul 21, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #71439 updated]

  • Start Time: 2020-07-21T15:14:46.100+0000

  • Duration: 4 min 20 sec

@mikecote mikecote added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 labels Jul 21, 2020
@mikecote mikecote marked this pull request as ready for review July 21, 2020 19:18
@mikecote mikecote requested review from a team as code owners July 21, 2020 19:18
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👏

So much better than before 😍

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From cases perspective LTGM!

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, made some comments

@@ -91,7 +101,7 @@ export class ActionTypeRegistry {
)
);
}
this.actionTypes.set(actionType.id, { ...actionType });
this.actionTypes.set(actionType.id, { ...actionType } as ActionType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I haven't gotten to the point where this cast is needed, but I woulda thought we could just do

this.actionTypes.set(actionType.id, actionType);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's to "cast" it to a generic, otherwise typescript complains that it can't convert Config, Secret and Params type to generics.

@@ -112,7 +122,11 @@ export class ActionTypeRegistry {
/**
* Returns an action type, throws if not registered
*/
public get(id: string): ActionType {
public get<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't gotten to the point where this would be needed, but wondering what the real use case is to have get() parameterized like this. No harm, just seems like it would only be used in unit tests for actions :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmmorris would know more, he helped me come up with the design. From what I saw, it's the only way we could get it to work as I don't know if you can make an interface extend a generic while also having sub-generic types. (CustomActionType extends ActionType, then setting Config, Secrets, Params generics).

@mikecote
Copy link
Contributor Author

well now I just feel bad/sad for having you add this typing, when it's mostly void and unknown 😭

This will be good for future work and eventually adding support to the existing ones :) We can eventually make the types explicit without providing default values for any of them. This will make new action types consider type safety.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

‼️ unable to find a baseline build for [master@de22caa]. Try merging the upstream branch and trying again.

History

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

@mikecote mikecote merged commit 54e13ad into elastic:master Aug 4, 2020
mikecote added a commit to mikecote/kibana that referenced this pull request Aug 4, 2020
* Initial attempt at making the actions plugin support generics

* Export WebhookMethods

* Fix typings for registry

* Usage of Record<string, unknown>

* Apply feedback from Gidi

* Cleanup

* Fix validate_with_schema

* Cleanup pt2

* Fix failing tests

* Add generics to ActionType for ActionTypeExecutorResult

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 5, 2020
* master: (74 commits)
  [Discover] Inline noWhiteSpace function (elastic#74331)
  [DOCS] Add Observability topic (elastic#73041)
  skip flaky suite (elastic#74327)
  [Security Solution][Detections] Fixes Severity Override not matching for Elastic Endpoint Security rule (elastic#74317)
  [Security Solution][Exceptions] - Fixes exceptions builder nested deletion issue and adds unit tests (elastic#74250)
  Fixed Alert details does not update page title and breadcrumb (elastic#74214)
  [src/dev/build] build Kibana Platform bundles from source (elastic#73591)
  [Reporting] Shorten asset path to help CLI FS Watcher (elastic#74185)
  Fix TMS not loaded in legacy maps (elastic#73570)
  [Security Solution] styling for notes' panel (elastic#74274)
  [Security Solution][Tech Debt] cleans up ts-ignore issues and some smaller linter issues  (elastic#74268)
  Make the actions plugin support generics (elastic#71439)
  [Security Solution] Keep original note creator (elastic#74203)
  [CI] Fix xpack kibana build dir in xpack visual regression script
  [CI] Fix baseline_capture job by adding parallel process number back
  [Monitoring] Ensure setup mode works on cloud but only for alerts (elastic#73127)
  [Maps] Custom color ramps should show correctly on the map for mvt layers (elastic#74169)
  [kbn/optimizer] remove unused modules (elastic#74195)
  [CI] Add pipeline task queue framework and merge workers into one (elastic#71268)
  Using msearch for tree api endpoint (elastic#73813)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 5, 2020
* master: (115 commits)
  [Logs UI] Correct trial period duration in anomaly splash screen (elastic#74249)
  [Discover] Inline noWhiteSpace function (elastic#74331)
  [DOCS] Add Observability topic (elastic#73041)
  skip flaky suite (elastic#74327)
  [Security Solution][Detections] Fixes Severity Override not matching for Elastic Endpoint Security rule (elastic#74317)
  [Security Solution][Exceptions] - Fixes exceptions builder nested deletion issue and adds unit tests (elastic#74250)
  Fixed Alert details does not update page title and breadcrumb (elastic#74214)
  [src/dev/build] build Kibana Platform bundles from source (elastic#73591)
  [Reporting] Shorten asset path to help CLI FS Watcher (elastic#74185)
  Fix TMS not loaded in legacy maps (elastic#73570)
  [Security Solution] styling for notes' panel (elastic#74274)
  [Security Solution][Tech Debt] cleans up ts-ignore issues and some smaller linter issues  (elastic#74268)
  Make the actions plugin support generics (elastic#71439)
  [Security Solution] Keep original note creator (elastic#74203)
  [CI] Fix xpack kibana build dir in xpack visual regression script
  [CI] Fix baseline_capture job by adding parallel process number back
  [Monitoring] Ensure setup mode works on cloud but only for alerts (elastic#73127)
  [Maps] Custom color ramps should show correctly on the map for mvt layers (elastic#74169)
  [kbn/optimizer] remove unused modules (elastic#74195)
  [CI] Add pipeline task queue framework and merge workers into one (elastic#71268)
  ...
mikecote added a commit that referenced this pull request Aug 5, 2020
* Initial attempt at making the actions plugin support generics

* Export WebhookMethods

* Fix typings for registry

* Usage of Record<string, unknown>

* Apply feedback from Gidi

* Cleanup

* Fix validate_with_schema

* Cleanup pt2

* Fix failing tests

* Add generics to ActionType for ActionTypeExecutorResult

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@mikecote mikecote linked an issue Aug 5, 2020 that may be closed by this pull request
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 5, 2020
* master: (154 commits)
  [ML] Fix initial plugin's bundle size (elastic#74047)
  [Ingest Manager] prevent crash on unhandled rejection from setupIngestManager (elastic#74300)
  [Logs UI] Correct trial period duration in anomaly splash screen (elastic#74249)
  [Discover] Inline noWhiteSpace function (elastic#74331)
  [DOCS] Add Observability topic (elastic#73041)
  skip flaky suite (elastic#74327)
  [Security Solution][Detections] Fixes Severity Override not matching for Elastic Endpoint Security rule (elastic#74317)
  [Security Solution][Exceptions] - Fixes exceptions builder nested deletion issue and adds unit tests (elastic#74250)
  Fixed Alert details does not update page title and breadcrumb (elastic#74214)
  [src/dev/build] build Kibana Platform bundles from source (elastic#73591)
  [Reporting] Shorten asset path to help CLI FS Watcher (elastic#74185)
  Fix TMS not loaded in legacy maps (elastic#73570)
  [Security Solution] styling for notes' panel (elastic#74274)
  [Security Solution][Tech Debt] cleans up ts-ignore issues and some smaller linter issues  (elastic#74268)
  Make the actions plugin support generics (elastic#71439)
  [Security Solution] Keep original note creator (elastic#74203)
  [CI] Fix xpack kibana build dir in xpack visual regression script
  [CI] Fix baseline_capture job by adding parallel process number back
  [Monitoring] Ensure setup mode works on cloud but only for alerts (elastic#73127)
  [Maps] Custom color ramps should show correctly on the map for mvt layers (elastic#74169)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Actions release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Alerting] Add Generics support to Alerting
8 participants