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.

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

[Response Ops][Rules] Version Get Rule Types API #195361

Merged
merged 17 commits into from
Oct 14, 2024

Conversation

jcger
Copy link
Contributor

@jcger jcger commented Oct 8, 2024

Summary

GET /api/alerting/rule_types item in #195181

@jcger jcger force-pushed the issue-187356-version-route-rule-types branch from 337b487 to e30a305 Compare October 8, 2024 13:25
default_action_group_id: schema.string(),
does_set_recovery_context: schema.maybe(schema.boolean()),
enabled_in_license: schema.boolean(),
fields_for_a_a_d: schema.maybe(schema.arrayOf(schema.string())),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that in main the field is returned as fieldsForAAD. I guess that would be a breaking change, should I leave it with snake_case or change it to work as in main?

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, it would be considered a breaking change so we would need to return fieldsForAAD.

import { TypesRulesResponseBodyV1 } from '../../../../../../../common/routes/rule/apis/list_types';

export const transformRuleTypesResponse = (
ruleTypes: Set<RegistryAlertTypeWithAuth>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I talked with @JiaweiWu about RegistryAlertTypeWithAuth Ideally it would be moved to be an application type but it's a complete mess. As long as it's not been used anywhere else, we should be fine. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it should be an application type and the rule client returns a more suitable structure. I would say it is out of the scope of this PR.

return {
...(ruleType.actionGroups ? { action_groups: ruleType.actionGroups } : {}),
...(ruleType.actionVariables ? { action_variables: ruleType.actionVariables } : {}),
...(ruleType.alerts ? { alerts: ruleType.alerts } : {}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one throws an error: Property 'alerts' does not exist on type 'RegistryAlertTypeWithAuth'. I'll add it to the type (see x-pack/plugins/alerting/server/rule_type_registry.ts changes), just let me know if you have more info/know a better option

@jcger jcger 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) v9.0.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 9, 2024
@jcger jcger marked this pull request as ready for review October 9, 2024 07:34
@jcger jcger requested a review from a team as a code owner October 9, 2024 07:34
@elasticmachine
Copy link
Contributor

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

@jcger jcger added v8.16.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 9, 2024
@cnasikas cnasikas self-requested a review October 9, 2024 14:47
@jcger
Copy link
Contributor Author

jcger commented Oct 9, 2024

/ci

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

LGTM! It amazes me that this is a public API that returns all this stuff. Maybe we should document only the important ones.

name: ruleType.name,
producer: ruleType.producer,
recovery_action_group: ruleType.recoveryActionGroup,
rule_task_timeout: ruleType.ruleTaskTimeout,
Copy link
Member

Choose a reason for hiding this comment

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

super nit: It seems that ruleTaskTimeout and defaultScheduleInterval can be undefined. Should we follow the same patter as above (...(ruleType.alerts ? { alerts: ruleType.alerts } : {}),)?

name: schema.string(),
});

export const typesRulesResponseBodySchema = schema.arrayOf(
Copy link
Member

Choose a reason for hiding this comment

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

I think the schema is missing default_schedule_interval.

…356-version-route-rule-types' of github.com:jcger/kibana into issue-187356-version-route-rule-types
does_set_recovery_context: schema.maybe(schema.boolean()),
enabled_in_license: schema.boolean(),
// eslint-disable-next-line @typescript-eslint/naming-convention
fields_for_AAD: schema.maybe(schema.arrayOf(schema.string())), // we cannot introduce breaking changes yet
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be fieldsForAAD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you so much for catching this one 😱

@jcger jcger enabled auto-merge (squash) October 14, 2024 12:04
@jcger jcger merged commit 512a31d into elastic:main Oct 14, 2024
42 of 43 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11330902192

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 14, 2024
## Summary

`GET /api/alerting/rule_types` item in
elastic#195181

(cherry picked from commit 512a31d)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

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 Oct 15, 2024
…6175)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Response Ops][Rules] Version Get Rule Types API
(#195361)](#195361)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Julian
Gernun","email":"17549662+jcger@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-10-14T15:46:17Z","message":"[Response
Ops][Rules] Version Get Rule Types API (#195361)\n\n##
Summary\r\n\r\n`GET /api/alerting/rule_types` item
in\r\nhttps://github.com//issues/195181","sha":"512a31d7a1e42139c2e1b26e961b2226ace3836d","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:ResponseOps","v9.0.0","backport:prev-minor","v8.16.0"],"title":"[Response
Ops][Rules] Version Get Rule Types
API","number":195361,"url":"https://github.com/elastic/kibana/pull/195361","mergeCommit":{"message":"[Response
Ops][Rules] Version Get Rule Types API (#195361)\n\n##
Summary\r\n\r\n`GET /api/alerting/rule_types` item
in\r\nhttps://github.com//issues/195181","sha":"512a31d7a1e42139c2e1b26e961b2226ace3836d"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195361","number":195361,"mergeCommit":{"message":"[Response
Ops][Rules] Version Get Rule Types API (#195361)\n\n##
Summary\r\n\r\n`GET /api/alerting/rule_types` item
in\r\nhttps://github.com//issues/195181","sha":"512a31d7a1e42139c2e1b26e961b2226ace3836d"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Julian Gernun <17549662+jcger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) 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.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants