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

[RAM][HTTP Versioning] Version Rule Bulk Edit Endpoint #161912

Merged
merged 7 commits into from
Jul 25, 2023
Merged

[RAM][HTTP Versioning] Version Rule Bulk Edit Endpoint #161912

merged 7 commits into from
Jul 25, 2023

Conversation

JiaweiWu
Copy link
Contributor

@JiaweiWu JiaweiWu commented Jul 13, 2023 •
edited
Loading

Summary

Resolves: #161395
Parent Issue: #157883

Adds versioned types to the rule bulk_edit endpoint.

This PR also moves around the folder structure slightly, by adding a sub folder for the data/route/application methods:

Before

image

After

image

Notice I added a methods folder to contain the methods, I did the same for the data and route folders as well. I think this improves the hierarchy of these modules, If folks are ok with it then I will update the doc with the new folder structure.

Checklist

@JiaweiWu 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.10.0 labels Jul 13, 2023
@JiaweiWu
Copy link
Contributor Author

@elasticmachine merge upstream

@JiaweiWu JiaweiWu marked this pull request as ready for review July 13, 2023 21:17
@JiaweiWu JiaweiWu requested a review from a team as a code owner July 13, 2023 21:17
@elasticmachine
Copy link
Contributor

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

guskovaue
guskovaue reviewed Jul 17, 2023
View reviewed changes
x-pack/plugins/alerting/common/routes/r_rule/index.ts
@@ -0,0 +1,12 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for information, what's r_rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stands for recurrence rule, Its basically a protocol of sorts for communicating schedules with recurrences, we use it for snooze and maintenance windows https://icalendar.org/iCalendar-RFC-5545/3-8-5-3-recurrence-rule.html

guskovaue
guskovaue reviewed Jul 17, 2023
View reviewed changes
x-pack/plugins/alerting/common/routes/rule/response/constants/latest.ts
@@ -0,0 +1,8 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

This question is not exactly for this file. I see that there is the apis folder with several api endpoints. And I see close to it folder response. Is it a response only for one endpoint? Which one? Not clear from naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rule response is like a shareable data schema for rules, since most of our rule endpoints will end up returning one or many rules, one would assume if the rule response schema changes for one, it would change for all of them. Hence why it's been extracted and re-used.

@JiaweiWu
Copy link
Contributor Author

@elasticmachine merge upstream

Zacqary
Zacqary requested changes Jul 19, 2023
View reviewed changes
x-pack/plugins/alerting/server/routes/index.ts Outdated
@@ -36,7 +36,7 @@ import { muteAlertRoute } from './mute_alert';
import { unmuteAllRuleRoute } from './unmute_all_rule';
import { unmuteAlertRoute } from './unmute_alert';
import { updateRuleApiKeyRoute } from './update_rule_api_key';
import { bulkEditInternalRulesRoute } from './bulk_edit_rules';
import { bulkEditInternalRulesRoute } from './rule/apis/buk_edit/bulk_edit_rules_route';
Copy link
Contributor

Choose a reason for hiding this comment

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

buk_edit -> bulk_edit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch 😆

Zacqary
Zacqary approved these changes Jul 24, 2023
View reviewed changes
Copy link
Contributor

@Zacqary Zacqary left a comment

Choose a reason for hiding this comment

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

Looks good!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
alerting 765 754 -11

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 48 47 -1
Unknown metric groups

API count

id before after diff
alerting 795 784 -11

History

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

@JiaweiWu JiaweiWu merged commit d4e6a19 into elastic:main Jul 25, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 25, 2023
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
@JiaweiWu @kibanamachine
[RAM][HTTP Versioning] Version Rule Bulk Edit Endpoint (elastic#161912)
ea64152 
## Summary

Resolves: elastic#161395
Parent Issue: elastic#157883

Adds versioned types to the rule `bulk_edit` endpoint.

This PR also moves around the folder structure slightly, by adding a sub
folder for the `data`/`route`/`application` methods:

## Before

![image](https://github.com/elastic/kibana/assets/74562234/3ae19871-cc5c-4180-b099-5fed86c3870b)

## After

![image](https://github.com/elastic/kibana/assets/74562234/bbd67bb4-4899-4f65-966e-64964f994a04)

Notice I added a `methods` folder to contain the methods, I did the same
for the `data` and `route` folders as well. I think this improves the
hierarchy of these modules, If folks are ok with it then I will update
the doc with the new folder structure.

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guskovaue guskovaue guskovaue left review comments

@jcger jcger jcger approved these changes

@Zacqary Zacqary Zacqary approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Alerting/RulesManagement Issues related to the Rules Management UX release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RAM][HTTP Versioning] Version Bulk Edit Endpoint
7 participants
@JiaweiWu @elasticmachine @kibana-ci @Zacqary @jcger @guskovaue @kibanamachine