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

Exposed common EuiExpressions to separate components be able to reuse for building new for Alert Types #56466

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Jan 31, 2020

Summary

#47758

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

@YulNaumenko YulNaumenko added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Jan 31, 2020
@YulNaumenko YulNaumenko self-assigned this Jan 31, 2020
@elasticmachine
Copy link
Contributor

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

…mon-alert-type-expressions

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
…mon-alert-type-expressions

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
…mon-alert-type-expressions

# Conflicts:
#	x-pack/legacy/plugins/triggers_actions_ui/np_ready/public/application/components/builtin_alert_types/threshold/expression.tsx
#	x-pack/legacy/plugins/triggers_actions_ui/np_ready/public/application/sections/alert_add/alert_add.tsx
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.

Looking good 👍

Just a few typing issue, but overall LGTM.

@@ -226,7 +230,7 @@ export const ThresholdVisualization: React.FunctionComponent<Props> = ({ alert }
const dateFormatter = (d: number) => {
return moment(d)
.tz(timezone)
.format(getTimeBuckets(alert.params).getScaledDateFormat());
.format(getTimeBuckets(alertParams).getScaledDateFormat());
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to avoid any and the as number[][][] here?

For the any: Looking at the underlying type, it looks like it uses number explicitly, so we shouldn't need to cast this here. 🤔

For the number[][][]: Looks the core issue is the any in getThresholdAlertVisualizationData, so can we perhaps validate the result of the API call in getThresholdAlertVisualizationData and avoid the any there too?

…mon-alert-type-expressions

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

LGTM! Just a few nits / comments.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@elastic elastic deleted a comment from kibanamachine Feb 6, 2020
@YulNaumenko YulNaumenko requested a review from a team as a code owner February 6, 2020 19:32
@elasticmachine
Copy link
Contributor

💔 Build Failed


Test Failures

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/plugins/spaces/server/lib/request_interceptors.onPostAuthInterceptor requests proxied to the legacy platform redirects to the space selector screen when accessing an app within a non-existent space

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 7 times on tracked branches: https://github.com/elastic/kibana/issues/55953


Stack Trace

: Timeout - Async callback was not invoked within the 30000ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 30000ms timeout specified by jest.setTimeout.Error: 
    at new Spec (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
    at new Spec (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/setup_jest_globals.js:80:9)
    at specFactory (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Env.js:575:24)
    at Env.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Env.js:644:24)
    at Env.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:132:23)
    at it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/jasmineLight.js:93:21)
    at Suite.describe (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts:233:5)
    at addSpecsToSuite (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Env.js:496:51)
    at Env.describe (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Env.js:466:11)
    at describe (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/jasmineLight.js:81:18)
    at Suite.Object.<anonymous>.describe (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts:232:3)
    at addSpecsToSuite (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Env.js:496:51)
    at Env.describe (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Env.js:466:11)
    at describe (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/jasmineLight.js:81:18)
    at Object.<anonymous> (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts:33:1)
    at Runtime._execModule (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runtime/build/index.js:867:68)
    at Runtime._loadModule (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runtime/build/index.js:577:12)
    at Runtime.requireModule (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runtime/build/index.js:433:10)
    at /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/index.js:202:13
    at Generator.next (<anonymous>)
    at asyncGeneratorStep (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/index.js:27:24)
    at _next (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/index.js:47:9)
    at /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/index.js:52:7
    at new Promise (<anonymous>)
    at /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/index.js:44:12
    at jasmine2 (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/index.js:60:19)
    at /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:385:24
    at Generator.next (<anonymous>)
    at asyncGeneratorStep (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:161:24)
    at _next (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:181:9)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/plugins/spaces/server/lib/request_interceptors.onPostAuthInterceptor requests proxied to the legacy platform when accessing the kibana app it always allows the request to continue

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 7 times on tracked branches: https://github.com/elastic/kibana/issues/55954


Stack Trace

Error: Http server is not setup up yet
    at HttpServer.start (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/src/core/server/http/http_server.ts:142:13)
    at HttpService.start (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/src/core/server/http/http_service.ts:134:29)

History

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

…mon-alert-type-expressions

# Conflicts:
#	x-pack/legacy/plugins/triggers_actions_ui/np_ready/public/application/components/builtin_alert_types/threshold/constants/aggregation_types.ts
#	x-pack/legacy/plugins/triggers_actions_ui/np_ready/public/application/components/builtin_alert_types/threshold/constants/comparators.ts
#	x-pack/legacy/plugins/triggers_actions_ui/np_ready/public/application/components/builtin_alert_types/threshold/constants/index.ts
#	x-pack/legacy/plugins/triggers_actions_ui/np_ready/public/application/components/builtin_alert_types/threshold/types.ts
#	x-pack/legacy/plugins/triggers_actions_ui/np_ready/public/application/lib/get_time_options.test.ts
#	x-pack/legacy/plugins/triggers_actions_ui/np_ready/public/application/lib/get_time_options.ts
#	x-pack/legacy/plugins/triggers_actions_ui/np_ready/public/application/lib/get_time_unit_label.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_alert_types/threshold/constants/aggregation_types.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_alert_types/threshold/constants/comparators.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_alert_types/threshold/constants/index.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_alert_types/threshold/types.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_alert_types/threshold/visualization.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/lib/get_time_options.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/get_time_options.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/get_time_unit_label.ts
#	x-pack/plugins/triggers_actions_ui/public/application/sections/action_connector_form/action_connector_form.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/sections/action_connector_form/action_type_menu.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/sections/action_connector_form/connector_add_modal.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/common/constants/index.ts
#	x-pack/plugins/triggers_actions_ui/public/common/expression_items/index.ts
#	x-pack/plugins/triggers_actions_ui/public/common/index.ts
#	x-pack/plugins/triggers_actions_ui/public/common/lib/get_time_options.test.ts
#	x-pack/plugins/triggers_actions_ui/public/common/lib/get_time_options.ts
#	x-pack/plugins/triggers_actions_ui/public/common/lib/get_time_unit_label.ts
#	x-pack/plugins/triggers_actions_ui/public/common/types.ts
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@YulNaumenko YulNaumenko merged commit 2e0d09b into elastic:master Feb 7, 2020
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 7, 2020
…b.com:jloleysens/kibana into console/feature/text-objects-in-saved-objects

* 'console/feature/text-objects-in-saved-objects' of github.com:jloleysens/kibana: (103 commits)
  fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998)
  TS of esKuery\node_types  (elastic#56857)
  Kibana app migration: Move static code dependencies into kibana_legacy plugin, part 1 (elastic#56408)
  Retry ES API calls that fail with 410/Gone (elastic#56950)
  [APM] Show missing permissions message to the user on the Services overview (elastic#56374)
  Fixing flaky CI tests for custom appRoutes (elastic#55763)
  [State Management][Docs] State syncing utils docs (elastic#56479)
  [Index management] Remove index mapper setting in tests (elastic#57066)
  Exposed common EuiExpressions to separate components be able to reuse for building new for Alert Types  (elastic#56466)
  [SIEM] update url state between page if date is relative (elastic#56813)
  fix for chart_types test (elastic#57056)
  chore(NA): remove compress from dll minimizer (elastic#57023)
  [File upload] Migrate routing to NP & add route validation (elastic#52313)
  Adding docs for grouped nav advanced setting (elastic#57013)
  Use i18n titles for field formatters, human names for numeral locales (elastic#56348)
  [Maps] Remove EMS catalogue url from docs (elastic#57020)
  [Endpoint] ERT-82 ERT-83 ERT-84: Alert list API with pagination (elastic#56538)
  [DOCS] Adds Apple notarization info to install doc (elastic#57042)
  [ML] New Platform server shim: update results service routes to use new platform router (elastic#56886)
  Fix typo on detection engine rule (elastic#56993)
  ...
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Feb 10, 2020
… for building new for Alert Types (elastic#56466)

* Exposed common Expression to separate components be able to reuse

* Expressions with unit tests

* Fixed type check

* Fixed merge issues

* Fixed due to review

* Cleaned up some not used params and added position popover definition

* fixed type check

* Unbinded alerting reusable components from application context

* Added consumer and alertTypeId with enable change alert type button props

* Fixed case for default alert type id was set

* Fixed chart visualization issues

* Exposed registry in triggers and actions ui

* Fixed alert_list to enable charts

* Fixed due to comments and simplified props
YulNaumenko added a commit that referenced this pull request Feb 10, 2020
… reuse for building new for Alert Types (#56466) (#57224)

* Exposed common EuiExpressions to separate components be able to reuse for building new for Alert Types  (#56466)

* Exposed common Expression to separate components be able to reuse

* Expressions with unit tests

* Fixed type check

* Fixed merge issues

* Fixed due to review

* Cleaned up some not used params and added position popover definition

* fixed type check

* Unbinded alerting reusable components from application context

* Added consumer and alertTypeId with enable change alert type button props

* Fixed case for default alert type id was set

* Fixed chart visualization issues

* Exposed registry in triggers and actions ui

* Fixed alert_list to enable charts

* Fixed due to comments and simplified props

* Fixed type check issue

* type check error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:Alerting 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.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants