[RAM][HTTP versioning] Version Bulk Enable Route #179778

JiaweiWu · 2024-04-01T22:34:13Z

Summary

Parent Issue: #157883
Issue: #179669

Versions the PATCH /internal/alerting/rules/_bulk_enable endpoint with added input and output validation.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2024-04-01T22:34:16Z

Pinging @elastic/response-ops (Team:ResponseOps)

JiaweiWu · 2024-04-01T22:34:28Z

@elasticmachine merge upstream

umbopepato

Tested bulk enabling with different rule types and can confirm that the endpoint works correctly, LGTM!

Screen.Recording.2024-04-09.at.16.15.37.mov

cnasikas

LGTM! I left some comments.

x-pack/plugins/alerting/server/routes/rule/apis/bulk_enable/bulk_enable_rules_route.test.ts

...alerting/server/routes/rule/apis/bulk_enable/transforms/transform_bulk_enable_response/v1.ts

cnasikas · 2024-04-18T07:38:58Z

...alerting/server/routes/rule/apis/bulk_enable/transforms/transform_bulk_enable_response/v1.ts

+  response: BulkEnableRulesResult<Params>
+): BulkEnableRulesResponseV1<RuleParamsV1>['body'] => {
+  return {
+    rules: response.rules.map((rule) => transformRuleToRuleResponseV1<Params>(rule as Rule)),


I noticed that before the attributes we were returning were camel case but now it is snake case. Is this considered a breaking change?

Nah we were always returning snake case, the transform was done at the route level thats all.

Unless I miss something, here x-pack/plugins/alerting/server/routes/bulk_enable_rules.ts we do body: { ...bulkEnableResults, // rest of stuff } where bulkEnableResults = await rulesClient.bulkEnableRules({ filter, ids });. The response from bulkEnableRules is in camel case so the route returns camel case. You can see it from the tests that we were returning camel case and now we are returning snake case. Given that the API is internal I think is fine. I just wanted to be sure we are not introducing a breaking change anywhere.

Yes thankfully we never use the return value of bulkEnableRules.

x-pack/plugins/alerting/server/application/rule/methods/bulk_enable/bulk_enable_rules.ts

...lugins/alerting/server/application/rule/methods/bulk_enable/types/bulk_enable_rules_types.ts

x-pack/plugins/alerting/server/application/rule/methods/bulk_enable/bulk_enable_rules.ts

cnasikas · 2024-04-18T07:53:36Z

x-pack/plugins/alerting/server/routes/rule/apis/bulk_enable/bulk_enable_rules_route.test.ts

@@ -208,7 +210,9 @@ describe('bulkEnableRulesRoute', () => {
      bulkEnableRulesRoute({ router, licenseState });
      const [_, handler] = router.patch.mock.calls[0];

-      rulesClient.bulkEnableRules.mockResolvedValueOnce(bulkEnableActionsResult);
+      rulesClient.bulkEnableRules.mockResolvedValueOnce(
+        bulkEnableActionsResult as unknown as BulkEnableRulesResult


Is it possible to avoid the casting here? If in the future we extend the response we will not get notified by TS and we may miss a bug.

Having to do this because we were so lazy with mocking rules in our tests and our types were so bad that it allowed it to work. Now that we are strict with our types these things start to surface.

I could fix it but i feel like thats out of the scope of this PR

I believe it is easier to do small fixes while we version rules. Otherwise, they will pile up to a point where it will be hard to do it and probably we will forget to do it. Fixing tests feels like part of this PR because we change the types and tests should follow. I am not advocating fixing everything just things that will help us prevent bugs in the future. If the fix involves fixing code unrelated to the rulesClient.bulkEnableRules like the issue you said about SanitizedRule then at least we can type the bulkEnableActionsResult and keep your casting so in the future will warn us that we need to take into account more attributes. Wdyt?

cnasikas

Tested and is working as expected. LGTM!

kibana-ci · 2024-04-29T08:33:57Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 05888cd

Failed CI Steps

Jest Tests #9

Test Failures

[job] [logs] Jest Tests #9 / usePostPushToService refresh the case after pushing

Metrics [docs]

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`alerting`	55	56	+1

History

💚 Build #204897 succeeded d54c6ec
💛 Build #204842 was flaky 02afccb
💔 Build #204841 failed 02afccb
💚 Build #204568 succeeded 17a2229
💔 Build #204526 failed 2e0118d
💛 Build #200422 was flaky ccfd927

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

kibanamachine · 2024-04-30T13:49:27Z

💔 All backports failed

Status	Branch	Result
❌	8.14	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 179778

Questions ?

Please refer to the Backport tool documentation

Version bulk enable api

daf48ae

JiaweiWu added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Alerting/RulesManagement Issues related to the Rules Management UX v8.14.0 labels Apr 1, 2024

JiaweiWu requested a review from a team as a code owner April 1, 2024 22:34

Merge branch 'main' into issue-179669-version-bulk-enable-api

a63862f

JiaweiWu mentioned this pull request Apr 1, 2024

[Rules] [Meta] Road to versioned HTTP APIs #157883

Closed

54 tasks

JiaweiWu added 2 commits April 1, 2024 17:04

Fix tests

c9b5edc

Fix tests

ccfd927

umbopepato approved these changes Apr 9, 2024

View reviewed changes

Fix merge conflicts

2e0118d

cnasikas reviewed Apr 18, 2024

View reviewed changes

JiaweiWu added 4 commits April 18, 2024 17:31

Fix tests

17a2229

Merge branch 'main' into issue-179669-version-bulk-enable-api

416c550

Address comments

02afccb

Remove generics

d54c6ec

cnasikas approved these changes Apr 23, 2024

View reviewed changes

Merge conflicts

05888cd

JiaweiWu linked an issue Apr 29, 2024 that may be closed by this pull request

[RAM] Version Bulk Enable API #179669

Closed

JiaweiWu merged commit 4b4c573 into elastic:main Apr 30, 2024
34 checks passed

kibanamachine added the v8.15.0 label Apr 30, 2024

JiaweiWu removed the v8.14.0 label Apr 30, 2024

kibanamachine added the backport:skip This commit does not require backporting label Apr 30, 2024

banderror mentioned this pull request Oct 10, 2024

[Security Solution] Cannot enable more than 1000 detection rules at once #195805

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RAM][HTTP versioning] Version Bulk Enable Route #179778

[RAM][HTTP versioning] Version Bulk Enable Route #179778

JiaweiWu commented Apr 1, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Apr 1, 2024

JiaweiWu commented Apr 1, 2024

umbopepato left a comment

cnasikas left a comment

cnasikas Apr 18, 2024

JiaweiWu Apr 18, 2024 •

edited

Loading

cnasikas Apr 18, 2024

JiaweiWu Apr 22, 2024

cnasikas Apr 18, 2024

JiaweiWu Apr 18, 2024 •

edited

Loading

cnasikas Apr 18, 2024 •

edited

Loading

cnasikas left a comment

kibana-ci commented Apr 29, 2024

kibanamachine commented Apr 30, 2024

[RAM][HTTP versioning] Version Bulk Enable Route #179778

[RAM][HTTP versioning] Version Bulk Enable Route #179778

Conversation

JiaweiWu commented Apr 1, 2024 • edited by kibanamachine Loading

Summary

Checklist

elasticmachine commented Apr 1, 2024

JiaweiWu commented Apr 1, 2024

umbopepato left a comment

Choose a reason for hiding this comment

cnasikas left a comment

Choose a reason for hiding this comment

cnasikas Apr 18, 2024

Choose a reason for hiding this comment

JiaweiWu Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

cnasikas Apr 18, 2024

Choose a reason for hiding this comment

JiaweiWu Apr 22, 2024

Choose a reason for hiding this comment

cnasikas Apr 18, 2024

Choose a reason for hiding this comment

JiaweiWu Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

cnasikas Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

cnasikas left a comment

Choose a reason for hiding this comment

kibana-ci commented Apr 29, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Public APIs missing exports

History

kibanamachine commented Apr 30, 2024

💔 All backports failed

Manual backport

Questions ?

JiaweiWu commented Apr 1, 2024 •

edited by kibanamachine

Loading

JiaweiWu Apr 18, 2024 •

edited

Loading

JiaweiWu Apr 18, 2024 •

edited

Loading

cnasikas Apr 18, 2024 •

edited

Loading