Fix update alert API to still work when AAD is out of sync #57039
Conversation
mikecote
added
Feature:Alerting
v8.0.0
release_note:skip
|
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
💔 Build FailedTo update your PR or re-run it, just comment with: |
💔 Build FailedHistory
To update your PR or re-run it, just comment with: |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
| } | ||
|
|
||
| const updateResult = await this.updateAlert({ id, data }, alertSavedObject); | ||
|
|
||
| await Promise.all([ |
There was a problem hiding this comment.
I think technically, we don't really need a Promise.all() here. The old key should be invalidated, but no one should really be dependent on when that happens. Especially when it's paired with the TM runNow(). But this is really a nit. Worthy of perhaps a TODO comment to separate these - will require a bit of work to do error-checking on the invalidate key, and what to do when it fails? Not worth fixing ATM, I don't think.
There was a problem hiding this comment.
I see your point, there's good and bad parts on both sides of doing an await here. My original thinking was that it facilitates tests knowing when the request is finished, all the underlying tasks are finished as well. This also facilitates controlling the flow in bulk if ever doing batches to avoid errors possibly caused by underlying tasks (ex: SIEM bulking requests for ~300 alerts).
On the other hand, it doesn't make sense slowing down the user and we have to add checks to make sure the error doesn't bubble up.
I'm happy to create an issue for this if we want to revisit this later?
There was a problem hiding this comment.
Let's wait to see if it causes some kind of perf issue. We should see these show up in APM traces, we'll get a sense for if there is something we'll need to change.
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…7039) * Ensure update API still works when AAD is broken * Add API integration test * Fix ESLint errors Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
…57305) * Ensure update API still works when AAD is broken * Add API integration test * Fix ESLint errors Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: (27 commits) Include actions new platform plugin for codeowners (elastic#57252) [APM][docs] 7.6 documentation updates (elastic#57124) Expressions refactor (elastic#54342) [ML] New Platform server shim: update annotation routes to use new platform router (elastic#57067) Remove injected ui app vars from Canvas (elastic#56190) update max_anomaly_score route schema to handle possible undefined values (elastic#57339) [Add panel flyout] Moving create new to the top of SavedObjectFinder (elastic#56428) Add mock of a legacy ui api to re-enable Canvas storybook (elastic#56673) [monitoring] Removes noisy event received log (elastic#57275) Remove use of copied MANAGEMENT_BREADCRUMBS and use `setBreadcrumbs` from management section's mount (elastic#57324) Advanced Settings management app to kibana platform plugin (elastic#56931) [ML] New Platform server shim: update recognize modules routes to use new platform router (elastic#57206) [ML] Fix overall stats for saved search on the Data Visualizer page (elastic#57312) [ML] [NP] Removing ui imports (elastic#56358) [SIEM] Fixes failing Cypress tests (elastic#57202) Create observability CODEOWNERS reference (elastic#57109) fix results service schema (elastic#57217) don't register a wrapper if browser side function exists. (elastic#57196) Ui Actions explorer example (elastic#57006) Fix update alert API to still work when AAD is out of sync (elastic#57039) ...
mikecote
removed
the
release_note:skip
Solves the update API for #56619.
In this PR, I'm allowing alerts to be updated when the AAD is out of sync. I'm also refactoring a bit the update unit tests of the alerts client.