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

[Alerting UI] Added ability to assign alert actions to resolved action group in UI #83139

Merged
merged 5 commits into from
Nov 15, 2020
Merged

[Alerting UI] Added ability to assign alert actions to resolved action group in UI #83139

merged 5 commits into from
Nov 15, 2020

Conversation

YulNaumenko
Copy link
Contributor

Resolve #82367

@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.11.0 labels Nov 11, 2020
@YulNaumenko YulNaumenko self-assigned this Nov 11, 2020
@YulNaumenko YulNaumenko marked this pull request as ready for review November 11, 2020 05:59
@elasticmachine
Copy link
Contributor

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

@YulNaumenko YulNaumenko mentioned this pull request Nov 11, 2020
Merged
ymao1
ymao1 approved these changes Nov 11, 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! I tested it locally with your other PR and saw the Resolved action group show up in in the dropdown.

x-pack/plugins/alerts/common/builtin_action_groups.ts Outdated
name: i18n.translate('xpack.alerts.builtinActionGroups.resolved', {
defaultMessage: 'Resolved',
}),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's in scope for this PR but now that we will have multiple action groups for each alert type, would it make sense for each action group to define what it's own default message is instead of having a defaultActionMessage as part of the alert type definition? That way the resolved action group could have a default message and we wouldn't need to separately define a resolvedActionGroupMessage.

Copy link
Contributor Author

@YulNaumenko YulNaumenko Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Each alert type may want to specify their own Resolution default message (as the default message for any other group) - make sense to me. Because it can contains variables with params and alert data for which instance it was resolved. We need to discuss how it can be possible (cc: @pmuellr )

Copy link
Member

Choose a reason for hiding this comment

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

thoughts on how to handle what the "default action message" is (and other fields like email subject), based on various things including action group, here: #66095 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pmuellr does it make sense to leave it as it is now till the #66095 (comment) ?

...s/triggers_actions_ui/public/application/sections/action_connector_form/action_type_form.tsx Outdated Show resolved Hide resolved
x-pack/plugins/triggers_actions_ui/public/application/lib/action_variables.ts Outdated Show resolved Hide resolved
@YulNaumenko YulNaumenko requested a review from a team as a code owner November 12, 2020 05:59
@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
triggersActionsUi 1.4MB 1.4MB +1.9KB

Page load bundle

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

id before after diff
triggersActionsUi 141.9KB 142.2KB +328.0B

History

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

@YulNaumenko YulNaumenko merged commit 228387c into elastic:master Nov 15, 2020
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Nov 15, 2020
@YulNaumenko
[Alerting UI] Added ability to assign alert actions to resolved actio…
92ac0b9 
…n group in UI (elastic#83139)

* Added ability to assign alert actions to resolved action group in UI

* Added unit test

* Fixed due to comments
@YulNaumenko YulNaumenko mentioned this pull request Nov 15, 2020
Merged
YulNaumenko added a commit that referenced this pull request Nov 15, 2020
@YulNaumenko
[Alerting UI] Added ability to assign alert actions to resolved actio…
73685c7 
…n group in UI (#83139) (#83389)

* Added ability to assign alert actions to resolved action group in UI

* Added unit test

* Fixed due to comments
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 16, 2020
@gmmorris
Merge branch 'master' into alerts/action-groups-as-conditions
c54458f 
* master:
  Migrate `/translations` route to core (elastic#83280)
  [APM] Ensure APM jest script can run (elastic#83398)
  [Uptime] Monitor status alert use url as instance (elastic#81736)
  [ML] Add basic license test run details to ML+Transform READMEs (elastic#83259)
  TSVB doesn't communicate it's index-patterns to dashboard (elastic#82964)
  [Alerting UI] Added ability to assign alert actions to resolved action group in UI (elastic#83139)
  Skips Vega test
  skip flaky suite (elastic#79967)
  [bundle optimization] Update to semver 7.x to get tree-shaking (elastic#83020)
  Added ability to fire actions when an alert instance is resolved (elastic#82799)
  [ML] Adds functional tests for the index data visualizer card contents (elastic#83174)
phillipb added a commit to phillipb/kibana that referenced this pull request Nov 16, 2020
@phillipb
Merge branch 'add-logs-to-node-details' of github.com:phillipb/kibana…
b58ece4 
… into add-logs-to-node-details

* 'add-logs-to-node-details' of github.com:phillipb/kibana:
  fix tall vislib charts in visualize (elastic#83340)
  [Lens] Avoid unnecessary data fetching on dimension flyout open (elastic#82957)
  [Security Solution][Case] Change case connector minimum required license to basic (elastic#83401)
  fix logstash central pipeline management test  (elastic#83281)
  [Search] Send to background UI (elastic#81793)
  Migrate `/translations` route to core (elastic#83280)
  [APM] Ensure APM jest script can run (elastic#83398)
  [Uptime] Monitor status alert use url as instance (elastic#81736)
  [ML] Add basic license test run details to ML+Transform READMEs (elastic#83259)
  TSVB doesn't communicate it's index-patterns to dashboard (elastic#82964)
  [Alerting UI] Added ability to assign alert actions to resolved action group in UI (elastic#83139)
  Skips Vega test
  skip flaky suite (elastic#79967)
  [bundle optimization] Update to semver 7.x to get tree-shaking (elastic#83020)
  Added ability to fire actions when an alert instance is resolved (elastic#82799)
@cnasikas cnasikas mentioned this pull request Nov 19, 2020
Closed
@mikecote mikecote added needs_docs release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Dec 16, 2020
@YulNaumenko YulNaumenko mentioned this pull request Feb 25, 2021
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmuellr pmuellr pmuellr left review comments

@ymao1 ymao1 ymao1 approved these changes

Assignees

@YulNaumenko YulNaumenko

Labels
Feature:Alerting needs_docs release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ability to assign alert actions to resolved action group in UI
6 participants
@YulNaumenko @elasticmachine @kibanamachine @pmuellr @ymao1 @mikecote