[Alert Summaries] [BE] Move “Notify When” and throttle from rule to action #144130
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Zacqary
added
Team:ResponseOps
…-ref HEAD~1..HEAD --fix'
…kibana into 143368-notify-migration
…kibana into 143368-notify-migration
…tion # Conflicts: # x-pack/plugins/alerting/server/routes/lib/index.ts
|
Pinging @elastic/response-ops (Team:ResponseOps) |
Zacqary
added
the
release_note:skip
XavierM
reviewed
Nov 2, 2022
XavierM
reviewed
Nov 2, 2022
XavierM
reviewed
Nov 2, 2022
XavierM
reviewed
Nov 2, 2022
banderror
reviewed
Nov 9, 2022
There was a problem hiding this comment.
Thank you for the fix @Zacqary. I posted a bunch of minor comments and nits.
I still have an open question/suggestion for the changes in security_solution/server/lib/detection_engine/rule_schema/model/rule_schemas.ts. Let's make a decision regarding this (I tend to revert the changes) and I'll approve the PR!
banderror
approved these changes
Nov 10, 2022
ersin-erdal
approved these changes
Nov 14, 2022
…tion # Conflicts: # x-pack/plugins/alerting/server/routes/create_rule.ts # x-pack/plugins/alerting/server/routes/lib/index.ts # x-pack/plugins/alerting/server/routes/update_rule.ts # x-pack/plugins/alerting/server/rules_client/rules_client.ts
guskovaue
reviewed
Nov 15, 2022
| @@ -0,0 +1,24 @@ | |||
| /* | |||
There was a problem hiding this comment.
It looks like total copy of x-pack/plugins/alerting/server/lib/validate_rule_type_params.ts. With the same variables, types.
Maybe better to reuse it?
There was a problem hiding this comment.
Ah looks like I committed this file by mistake
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Nov 16, 2022
…ction (elastic#144130) * Add frequency props to actions and validate them on edit/create * Nullify undefined throttle * Update schema to allow for frequency param on actions * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * Commit missing file * Fix types * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * Fix types * Fix jest * Fix validating global freq params * Add tests for create and edit * Reset legacy api * Make notify and throttle optional in route schemas * Fix tests * Split missing frequency test cases Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 0c864fe)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Nov 16, 2022
…e to action (#144130) (#145469) # Backport This will backport the following commits from `main` to `8.6`: - [[Alert Summaries] [BE] Move “Notify When” and throttle from rule to action (#144130)](#144130) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Zacqary Adam Xeper","email":"Zacqary@users.noreply.github.com"},"sourceCommit":{"committedDate":"2022-11-16T22:02:36Z","message":"[Alert Summaries] [BE] Move “Notify When” and throttle from rule to action (#144130)\n\n* Add frequency props to actions and validate them on edit/create\r\n\r\n* Nullify undefined throttle\r\n\r\n* Update schema to allow for frequency param on actions\r\n\r\n* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'\r\n\r\n* Commit missing file\r\n\r\n* Fix types\r\n\r\n* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'\r\n\r\n* Fix types\r\n\r\n* Fix jest\r\n\r\n* Fix validating global freq params\r\n\r\n* Add tests for create and edit\r\n\r\n* Reset legacy api\r\n\r\n* Make notify and throttle optional in route schemas\r\n\r\n* Fix tests\r\n\r\n* Split missing frequency test cases\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"0c864fe479e332aa92bf189f0123661cd52262ae","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:ResponseOps","Feature:Alerting/RulesManagement","v8.6.0","v8.7.0"],"number":144130,"url":"https://github.com/elastic/kibana/pull/144130","mergeCommit":{"message":"[Alert Summaries] [BE] Move “Notify When” and throttle from rule to action (#144130)\n\n* Add frequency props to actions and validate them on edit/create\r\n\r\n* Nullify undefined throttle\r\n\r\n* Update schema to allow for frequency param on actions\r\n\r\n* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'\r\n\r\n* Commit missing file\r\n\r\n* Fix types\r\n\r\n* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'\r\n\r\n* Fix types\r\n\r\n* Fix jest\r\n\r\n* Fix validating global freq params\r\n\r\n* Add tests for create and edit\r\n\r\n* Reset legacy api\r\n\r\n* Make notify and throttle optional in route schemas\r\n\r\n* Fix tests\r\n\r\n* Split missing frequency test cases\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"0c864fe479e332aa92bf189f0123661cd52262ae"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/144130","number":144130,"mergeCommit":{"message":"[Alert Summaries] [BE] Move “Notify When” and throttle from rule to action (#144130)\n\n* Add frequency props to actions and validate them on edit/create\r\n\r\n* Nullify undefined throttle\r\n\r\n* Update schema to allow for frequency param on actions\r\n\r\n* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'\r\n\r\n* Commit missing file\r\n\r\n* Fix types\r\n\r\n* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'\r\n\r\n* Fix types\r\n\r\n* Fix jest\r\n\r\n* Fix validating global freq params\r\n\r\n* Add tests for create and edit\r\n\r\n* Reset legacy api\r\n\r\n* Make notify and throttle optional in route schemas\r\n\r\n* Fix tests\r\n\r\n* Split missing frequency test cases\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"0c864fe479e332aa92bf189f0123661cd52262ae"}}]}] BACKPORT--> Co-authored-by: Zacqary Adam Xeper <Zacqary@users.noreply.github.com>
benakansara
pushed a commit
to benakansara/kibana
that referenced
this pull request
Nov 17, 2022
…ction (elastic#144130) * Add frequency props to actions and validate them on edit/create * Nullify undefined throttle * Update schema to allow for frequency param on actions * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * Commit missing file * Fix types * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * Fix types * Fix jest * Fix validating global freq params * Add tests for create and edit * Reset legacy api * Make notify and throttle optional in route schemas * Fix tests * Split missing frequency test cases Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
|
Cross-posting FYI @Zacqary, it looks like a cascade impact on Terraform was missed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #143368
notify_whenandthrottlenotify_whenandthrottleoptionalfrequencyprop in actions including a booleansummaryplusnotify_whenandthrottlenullinnotify_whenandthrottlewhen per-action configs are usedThis does NOT automatically migrate global
notify_whento per-actionfrequencyparams. Until #143376 merges, globalnotify_whenis still required to actually successfully send notifications.Checklist