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.

Sign up for GitHub

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

Jump to bottom

Removed a possibility to define two different names for Alert types on API and UI level. #86236

Merged
merged 8 commits into from
Dec 20, 2020
Merged

Removed a possibility to define two different names for Alert types on API and UI level. #86236

merged 8 commits into from
Dec 20, 2020

Conversation

YulNaumenko
Copy link
Contributor

Resolve #61695

@YulNaumenko YulNaumenko added Feature:Alerting v8.0.0 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.12.0 labels Dec 17, 2020
@YulNaumenko YulNaumenko self-assigned this Dec 17, 2020
@YulNaumenko YulNaumenko requested review from a team as code owners December 17, 2020 05:54
@YulNaumenko YulNaumenko requested a review from a team December 17, 2020 05:54
@YulNaumenko YulNaumenko requested a review from a team as a code owner December 17, 2020 05:54
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Dec 17, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@mikecote mikecote self-requested a review December 17, 2020 12:50
mikecote
mikecote reviewed Dec 17, 2020
View reviewed changes
x-pack/plugins/triggers_actions_ui/public/types.ts Outdated
@@ -197,6 +196,7 @@ export interface AlertTypeModel<AlertParamsType = any> {
| React.LazyExoticComponent<ComponentType<AlertTypeParamsExpressionProps<AlertParamsType>>>;
requiresAppContext: boolean;
defaultActionMessage?: string;
name?: JSX.Element;
Copy link
Contributor

Choose a reason for hiding this comment

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

I took a look at the places that still use this name property. It seems they're using it for string values. Can these be pulled from the server side now with the changes you've done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've just missed some of the places, but fortunately we have a great tests coverage to catch this as well!

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw one place using <FormattedMessage ... />. I think we'll be able to remove this line after the cleanup?

Suggested change
name?: JSX.Element;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I didn't realized at first it is a FormattedMessage :-)

chrisronline
chrisronline approved these changes Dec 17, 2020
View reviewed changes
Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM for Stack Monitoring

@YulNaumenko YulNaumenko requested a review from a team as a code owner December 17, 2020 17:20
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Dec 17, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

ogupte
ogupte approved these changes Dec 17, 2020
View reviewed changes
Copy link
Contributor

@ogupte ogupte left a comment

Choose a reason for hiding this comment

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

APM change LGTM!

mikecote
mikecote approved these changes Dec 17, 2020
View reviewed changes
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.

Changes LGTM!

ymao1
ymao1 approved these changes Dec 18, 2020
View reviewed changes
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM!

andrewvc
andrewvc approved these changes Dec 18, 2020
View reviewed changes
Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
monitoring 979.5KB 979.0KB -505.0B
triggersActionsUi 1.6MB 1.6MB -267.0B
uptime 1.0MB 1.0MB +12.0B
total -760.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
apm 49.2KB 48.6KB -586.0B
infra 180.2KB 179.8KB -405.0B
stackAlerts 17.3KB 16.9KB -449.0B
uptime 24.7KB 24.3KB -399.0B
total -1.8KB

History

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

@YulNaumenko YulNaumenko merged commit 396018f into elastic:master Dec 20, 2020
@YulNaumenko YulNaumenko mentioned this pull request Dec 20, 2020
Merged
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Dec 20, 2020
@YulNaumenko
Removed a possibility to define two different names for Alert types o…
cefbb33 
…n API and UI level. (elastic#86236)

* Removed a possibility to define two different names for Alert types on API and UI level

* fixed typechecks

* fixed typechecks

* fixed due to comments

* fixed typechecks

* fixed jest tests

* fixed typechecks
YulNaumenko added a commit that referenced this pull request Dec 20, 2020
@YulNaumenko
Removed a possibility to define two different names for Alert types o…
4737c73 
…n API and UI level. (#86236) (#86605)

* Removed a possibility to define two different names for Alert types on API and UI level

* fixed typechecks

* fixed typechecks

* fixed due to comments

* fixed typechecks

* fixed jest tests

* fixed typechecks
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 21, 2020
@gmmorris
Merge branch 'master' into alerts/remove-generics-defaults-redux
561ebb1 
* master: (48 commits)
  Fix request with disabled aggregation (elastic#85696)
  [Security Solution][Detections][Threshold Rules] Threshold Rule Bug Fixes (elastic#84918)
  Removed a possibility to define two different names for Alert types on API and UI level. (elastic#86236)
  Bump Node.js from version 14.15.2 to 14.15.3 (elastic#86593)
  [index patterns] Fleep app - Keep saved object field list until field caps provides fields (elastic#85370)
  [Security Solutions] fix timeline tabs + layout (elastic#86581)
  Upgrade to hapi version 20 (elastic#85406)
  App Services: Remove remaining uiActions, expressions, data, embeddable circular dependencies. (elastic#82791)
  Rename chartLibrary setting to legacyChartsLibrary (elastic#86529)
  [CI] TeamCity updates (elastic#85843)
  [Maps] Use Json for mvt-tests (elastic#86492)
  [Rollup Jobs] Added autofocus to cron editor (elastic#86324)
  [Monitoring][Alerting] CCR read exceptions alert (elastic#85908)
  [CI] Bump memory for main CI workers (elastic#86541)
  Explicitly set Elasticsearch heap size during CI and local development (elastic#86513)
  [App Search] Updates to results on the documents view (elastic#86181)
  [Discover] Change default sort handling  (elastic#85561)
  [App Search] Convert DocumentCreationModal to DocumentCreationFlyout (elastic#86508)
  [App Search] Sample Engines should have access to the Crawler (elastic#86502)
  Fixed duplication of create new modal (elastic#86489)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ymao1 ymao1 ymao1 approved these changes

@mikecote mikecote mikecote approved these changes

@ogupte ogupte ogupte approved these changes

@andrewvc andrewvc andrewvc approved these changes

@chrisronline chrisronline chrisronline approved these changes

Assignees

@YulNaumenko YulNaumenko

Labels
Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Alert types can have two names
8 participants
@YulNaumenko @elasticmachine @kibanamachine @chrisronline @andrewvc @ogupte @mikecote @ymao1