[alerting] removes usage of any throughout Alerting Services code #64161

gmmorris · 2020-04-22T11:05:26Z

Summary

This removes unneeded use of any throughout:

alerting
alerting_builtin
actions
task manager
event log

It also adds a linting rule that will prevent us from adding more any in the future unless an explicit exemption is made.

closes #64145

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~~
~~Documentation was added for features that require explanation or tutorials~~
~~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, including a check against IE11~~

For maintainers

This was checked for breaking API changes and was labeled appropriately

* master: (29 commits) [Dashboard] Deangularize navbar, attempt nr. 2 (elastic#61611) refactor action filter creation utils (elastic#62969) Refresh index pattern list before redirecting (elastic#63329) [APM]fixing custom link unit tests (elastic#64045) [Ingest] EPM & Fleet are enabled when Ingest is enabled (elastic#64103) [Alerting] Fixed bug with no possibility to edit the index name after adding (elastic#64033) [Maps] Map settings: min and max zoom (elastic#63714) [kbn-storybook] Use raw loader for text files (elastic#64108) [EPM] /packages/{package} endpoint to support upgrades (elastic#63629) [SIEM] New Platform Saved Objects Registration (elastic#64029) [Endpoint] Hook to handle events needing navigation via Router (elastic#63863) Fixed small issue in clone functionality (elastic#64085) [Endpoint]EMT-146: use ingest agent for status info (elastic#63921) [SIEM] Server NP Followup (elastic#64010) Register uiSettings on New Platform (elastic#64015) [Reporting] Integration polling config with client code (elastic#63754) [Docs]7.7 SIEM doc updates (elastic#63951) [SIEM] [Cases] Tags suggestions (elastic#63878) Include datasource UUID in agent config yaml, adjust overflow height of yaml view (elastic#64027) [DOCS] Add file size setting for Data Visualizer (elastic#64006) ...

gmmorris · 2020-04-22T11:11:05Z

x-pack/plugins/actions/server/builtin_action_types/pagerduty.ts

+  const data: {
+    event_action: ActionParamsType['eventAction'];
+    dedup_key: string;
+    payload?: {
+      summary: string;
+      source: string;
+      severity: string;
+      timestamp?: string;
+      component?: string;
+      group?: string;
+      class?: string;
+    };
+  } = {


Not that this is a change strictly to typing, not actual object, so compile-time addition, not runtime.

gmmorris · 2020-04-22T11:14:16Z

x-pack/plugins/actions/server/lib/validate_with_schema.ts

+  value: unknown
+): Record<string, unknown> {
+  if (actionType.validate) {
+    let name;
+    try {
+      switch (key) {
+        case 'params':
+          name = 'action params';
+          if (actionType.validate.params) {
+            return actionType.validate.params.validate(value);
+          }
+          break;
+        case 'config':
+          name = 'action type config';
+          if (actionType.validate.config) {
+            return actionType.validate.config.validate(value);
+          }
+
+          break;
+        case 'secrets':
+          name = 'action type secrets';
+          if (actionType.validate.secrets) {
+            return actionType.validate.secrets.validate(value);
+          }
+          break;
+        default:
+          // should never happen, but left here for future-proofing
+          throw new Error(`invalid actionType validate key: ${key}`);
+      }
+    } catch (err) {
+      // we can't really i18n this yet, since the err.message isn't i18n'd itself
+      throw Boom.badRequest(`error validating ${name}: ${err.message}`);
    }
-  } catch (err) {
-    // we can't really i18n this yet, since the err.message isn't i18n'd itself
-    throw Boom.badRequest(`error validating ${name}: ${err.message}`);
  }

-  // should never happen, but left here for future-proofing
-  throw new Error(`invalid actionType validate key: ${key}`);
+  return value as Record<string, unknown>;


This function was refactored to avoid having to do the as Record<string, unknown> casting 4 times in a single function, the logic is identical.

gmmorris · 2020-04-22T11:15:23Z

x-pack/plugins/actions/server/plugin.test.ts

+      // coreMock.createSetup doesn't support Plugin generics
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      await plugin.setup(coreSetup as any, pluginsSetup);


I'll open an issue with Platform - sadly this piece of generics typing was missed in our core mocks

gmmorris · 2020-04-22T11:17:38Z

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

+  // This will have to remain `any` until we can extend Action Executors with generics
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  config: Record<string, any>;
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  secrets: Record<string, any>;
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any


An issue has been added to address this: #64147

gmmorris · 2020-04-22T11:19:25Z

x-pack/plugins/alerting/server/test_utils/index.ts

+  let resolve: (arg: T) => void;
+  return Object.assign(new Promise<T>(r => (resolve = r)), {
+    resolve(arg: T) {
+      return setTimeout(() => resolve(arg), 0);
+    },
+  });


Removing an any in a test that was using resolvable revealed the typing was wrong here.

gmmorris · 2020-04-22T11:25:24Z

x-pack/plugins/task_manager/server/task.ts

+export type SerializedConcreteTaskInstance = Omit<
+  ConcreteTaskInstance,
+  'state' | 'params' | 'scheduledAt' | 'startedAt' | 'retryAt' | 'runAt'
+> & {
+  state: string;
+  params: string;
+  scheduledAt: string;
+  startedAt: string | null;
+  retryAt: string | null;
+  runAt: string;
+};


removing any revealed that the typing in our serialisation was wrong - this adds a missing type that correctly types the serialised shape.

gmmorris · 2020-04-22T11:29:28Z

x-pack/plugins/task_manager/server/task_store.ts

+    const c = savedObjectToConcreteTaskInstance(
+      // The SavedObjects update api forces a Partial on the `attributes` on the response,
+      // but actually returns the whole object that is passed to it, so as we know we're
+      // passing in the whole object, this is safe to do.
+      // This is far from ideal, but unless we change the SavedObjectsClient this is the best we can do
+      // (updatedSavedObject as unknown) as SavedObject<SerializedConcreteTaskInstance>
+      { ...updatedSavedObject, attributes: defaults(updatedSavedObject.attributes, attributes) }
+    );
+    return c;


This is what we discussed on the call yesterday - the typing in SOs basically forces a Partial<> where it isn't needed.
In case SOs changes it's underlying implementation in the future, we use defaults to ensure we still have all the required fields after update is applied.

elasticmachine · 2020-04-22T11:31:01Z

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

pmuellr · 2020-04-23T14:59:36Z

About 1/3 of the way through the review - looks great!

pmuellr

LGTM - I think all my comments were about comments :-)

pmuellr · 2020-04-23T04:23:24Z

x-pack/plugins/alerting/server/test_utils/index.ts

+  let resolve: (arg: T) => void;
+  return Object.assign(new Promise<T>(r => (resolve = r)), {
+    resolve(arg: T) {
+      return setTimeout(() => resolve(arg), 0);


do we need the setTimeout() here? I doubt it hurts anything, just adds a little more latency, who knows might be useful :-), but I'm curious

I don't think so tbh, I noticed that too, but decided to limit changes in this PR to things related to the removal of any :)

pmuellr · 2020-04-23T19:12:12Z

x-pack/plugins/alerting/server/types.ts

@@ -29,6 +36,8 @@ declare module 'src/core/server' {
 }

 export interface Services {
+  // This will have to remain `any` until we can extend Alert Services with generics


Is this comment correct? I thought the anys for callCluster was because callCluster definition, not because of our own generics.

Nope, mistake, good catch :)

Actually... looking at it I've remembered why I marked it that way - this is our exposed interface to the plugins and we could exposed it as unknown if we wanted to. Exposing as any allows plugins to avoid explicitly casting, which means they're more likely to drag a lack of typechecking into their code (any is pervasive, if they even reference this response (in the Promise<any>) then anything referenced from it will not be type checked, even though it might look like it is.

If we expose as unknown they would have to explicitly state what type they think it is and it will typecheck properly from there out.

But changing that interface isn't obvious and we'd have to look into it further.

pmuellr · 2020-04-23T19:22:18Z

x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/indices.ts

@@ -102,12 +103,14 @@ async function getIndicesFromPattern(
      },
    },
  };
-  const response = await dataClient.callAsCurrentUser('search', params);
-  if (response.status === 404 || !response.aggregations) {
+  const response: SearchResponse<unknown> = await dataClient.callAsCurrentUser('search', params);


ya, this client gets hard to reason about, as there are options to have it return different things on 404's, and what not - it's not even all just the client, ES has such options you can send on requests. Leaving the TODO comment is good for next time we need to look into this particular site ...

pmuellr · 2020-04-23T19:23:47Z

x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/routes/indices.ts

@@ -137,3 +140,9 @@ async function getAliasesFromPattern(

  return result;
 }
+


I like having these little stand-alone interfaces like this, for cases like this.

pmuellr · 2020-04-23T19:29:05Z

x-pack/plugins/event_log/server/es/cluster_client_adapter.ts

@@ -5,6 +5,7 @@
 */

 import { reject, isUndefined } from 'lodash';
+import { SearchResponse, Client } from 'elasticsearch';


Now I feel slightly embarrassed that I didn't do the basic typing when I wrote most of this code. Thanks for upgrading all this stuff!!!

hah nah, don't be silly, we did it to ourselves by not having this rule in place in the first place :)

x-pack/plugins/task_manager/server/task.ts

pmuellr · 2020-04-23T19:34:22Z

x-pack/plugins/task_manager/server/task_store.test.ts

@@ -3,6 +3,7 @@
 * or more contributor license agreements. Licensed under the Elastic License;
 * you may not use this file except in compliance with the Elastic License.
 */
+/* eslint-disable  @typescript-eslint/no-explicit-any */


probably don't need this anymore?

Good catch :)

YulNaumenko

LGTM

* master: (70 commits) KQL removes leading zero and breaks query (elastic#62748) [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193) [ML] Changes transforms wizard UI text (elastic#64150) [Alerting] change server log action type .log to .server-log in README (elastic#64124) [Metrics UI] Design Refresh: Inventory View, Episode 1 (elastic#64026) chore(NA): reduce siem bundle size using babel-plugin-transfor… (elastic#63269) chore(NA): use core-js instead of babel-polyfill on canvas sha… (elastic#63486) skip flaky suite (elastic#61173) skip flaky suite (elastic#62497) Renamed ilm policy for event log so it is not prefixed with dot (elastic#64262) [eslint] no_restricted_paths config cleanup (elastic#63741) Add Oil Rig Icon from @elastic/maki (elastic#64364) [Maps] Migrate Maps embeddables to NP (elastic#63976) [Ingest] Data streams list page (elastic#64134) chore(NA): add file-loader into jest moduleNameMapper (elastic#64330) [DOCS] Added images to automating report generation (elastic#64333) [SIEM][CASE] Api Integration Tests: Configuration (elastic#63948) Expose ability to check if API Keys are enabled (elastic#63454) [DOCS] Fixes formatting in alerting doc (elastic#64338) [data.search.aggs]: Create agg types function for terms agg. (elastic#63541) ...

kibanamachine · 2020-04-24T15:56:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 28d29f7

History

💔 Build #42996 failed 7b89039
💚 Build #42689 succeeded 2e22429
💔 Build #42679 failed 5565c98
💔 Build #42583 failed 8a7e27c
💔 Build #42409 failed e5257b3

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

…astic#64161) This removes unneeded use of `any` throughout: 1. alerting 2. alerting_builtin 3. actions 4. task manager 5. event log It also adds a linting rule that will prevent us from adding more `any` in the future unless an explicit exemption is made.

…4161) (#64441) This removes unneeded use of `any` throughout: 1. alerting 2. alerting_builtin 3. actions 4. task manager 5. event log It also adds a linting rule that will prevent us from adding more `any` in the future unless an explicit exemption is made.

…bana into ingest-node-pipeline/open-flyout-create-edit * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (116 commits) [Ingest Node Pipelines] More lenient treatment of on-failure value (elastic#64411) Report Deletion via UI- functional test (elastic#64031) Bump handlebars dependency from 4.5.3 to 4.7.6 (elastic#64402) [Uptime] Update TLS settings (elastic#64111) [alerting] removes usage of any throughout Alerting Services code (elastic#64161) [CANVAS] Moves notify to a canvas service (elastic#63268) [Canvas] Misc NP Stuff (elastic#63703) update apm index pattern (elastic#64232) Task/hostlist pagination (elastic#63722) [NP] Vega migration (elastic#63849) Move ensureDefaultIndexPattern into data plugin (elastic#63100) [Fleet] Fix agent status count to not include unenrolled agents (elastic#64106) Migrate graph_workspace saved object registration to Kibana platform (elastic#64157) Index pattern management UI -> TypeScript and New Platform Ready (edit_index_pattern) (elastic#64184) [ML] EuiDataGrid ml/transform components. (elastic#63447) [ML] Moving to kibana capabilities (elastic#64057) Move input_control_vis into NP (elastic#63333) remove reference to local application service in graph (elastic#64288) KQL removes leading zero and breaks query (elastic#62748) [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193) ...

gmmorris added 5 commits April 22, 2020 12:01

replace any with unknown in actions

55111fa

replace any with unknown in alerting

bf81639

replace any with unknown in task manager

78d11a7

replace any with unknown in event log

61d9018

gmmorris commented Apr 22, 2020

View reviewed changes

gmmorris added 3 commits April 22, 2020 12:21

fixed typo

8b757c4

removed mistaked change

f79e146

removed unneeded ts-ignore

4a51480

gmmorris commented Apr 22, 2020

View reviewed changes

gmmorris added 3 commits April 22, 2020 12:25

typo

90d9cd2

fixed comments

cca699e

removed unneeded var

e5257b3

gmmorris commented Apr 22, 2020

View reviewed changes

gmmorris added Feature:Actions Feature:Alerting Feature:Task Manager release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Apr 22, 2020

gmmorris marked this pull request as ready for review April 22, 2020 11:31

gmmorris requested a review from a team as a code owner April 22, 2020 11:31

gmmorris added v7.8.0 v8.0.0 labels Apr 22, 2020

gmmorris added 2 commits April 22, 2020 21:03

removed usage of anywhere possible

b3e668e

removed usage of anywhere possible

8a7e27c

gmmorris added 2 commits April 23, 2020 10:24

fixed some more missed anys

5565c98

corrected typing in alerting test

2e22429

pmuellr approved these changes Apr 23, 2020

View reviewed changes

YulNaumenko approved these changes Apr 23, 2020

View reviewed changes

gmmorris added 3 commits April 24, 2020 09:09

fixed typing in TaskStore

44439ed

fixed broken merge

28d29f7

gmmorris merged commit a012ddf into elastic:master Apr 24, 2020

gmmorris mentioned this pull request Apr 24, 2020

[7.x] [alerting] removes usage of any throughout Alerting Services code (#64161) #64441

Merged

gmmorris mentioned this pull request May 12, 2020

[Alerting] disable any in the Triggers UI plugin #66256

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[alerting] removes usage of any throughout Alerting Services code #64161

[alerting] removes usage of any throughout Alerting Services code #64161

gmmorris commented Apr 22, 2020 •

edited

Loading

gmmorris Apr 22, 2020

gmmorris Apr 22, 2020

gmmorris Apr 22, 2020

gmmorris Apr 22, 2020

gmmorris Apr 22, 2020

gmmorris Apr 22, 2020

gmmorris Apr 22, 2020

elasticmachine commented Apr 22, 2020

pmuellr commented Apr 23, 2020

pmuellr left a comment

pmuellr Apr 23, 2020

gmmorris Apr 24, 2020

pmuellr Apr 23, 2020

gmmorris Apr 24, 2020

gmmorris Apr 24, 2020

pmuellr Apr 23, 2020

gmmorris Apr 24, 2020

pmuellr Apr 23, 2020

pmuellr Apr 23, 2020

gmmorris Apr 24, 2020

pmuellr Apr 23, 2020

gmmorris Apr 24, 2020

YulNaumenko left a comment

kibanamachine commented Apr 24, 2020

		@@ -137,3 +140,9 @@ async function getAliasesFromPattern(

		return result;
		}

[alerting] removes usage of any throughout Alerting Services code #64161

[alerting] removes usage of any throughout Alerting Services code #64161

Conversation

gmmorris commented Apr 22, 2020 • edited Loading

Summary

Checklist

For maintainers

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

elasticmachine commented Apr 22, 2020

pmuellr commented Apr 23, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YulNaumenko left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 24, 2020

💚 Build Succeeded

History

gmmorris commented Apr 22, 2020 •

edited

Loading