Add rrule schema to task schedule with intermediate release by ersin-erdal · Pull Request #219429 · elastic/kibana

ersin-erdal · 2025-04-28T16:03:40Z

Towards: #216308

This PR adds the rrule schema to the task SO with an intermediate release for the following PR:
#217728

…te --fix'

… src/core/server/integration_tests/ci_checks'

elasticmachine · 2025-04-28T20:05:42Z

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

ymao1 · 2025-04-29T00:12:21Z

x-pack/platform/plugins/shared/task_manager/server/saved_objects/schemas/rrule.ts

+
+const rruleCommon = schema.object({
+  freq: schema.number(),
+  interval: schema.number(),


should we set a minimum here?

ymao1 · 2025-04-29T00:19:00Z

x-pack/platform/plugins/shared/task_manager/server/saved_objects/schemas/rrule.ts

+const byminute = schema.maybe(schema.arrayOf(schema.number({ min: 0, max: 59 })));
+const byhour = schema.maybe(schema.arrayOf(schema.number({ min: 0, max: 23 })));
+const byweekday = schema.maybe(schema.arrayOf(schema.number({ min: 1, max: 7 })));
+const bymonthday = schema.maybe(schema.arrayOf(schema.number({ min: 1, max: 31 })));


The alerting schema has some validations for these fields (checking for non-empty arrays). Should we add similar validations here? Also, I know we're only supporting a subset of the rRule specification, but should we try to make it as close to the alerting one as possible? For example, here, byweekday is a number that maps to an enum but in the alerting spec, it's a string.

Alerting schema: x-pack/platform/plugins/shared/alerting/common/routes/r_rule/request/schemas/v1.ts

I added the min length check but byweekday is interesting as it can take both string and number.
here: x-pack/platform/plugins/shared/alerting/server/saved_objects/schemas/raw_rule/v3.ts

validation in the request schema you shared also accepts numbers between 1-4 in the regex but as string!
Should we stick with it??

Btw idk why it is only between 1 and 4! maybe someone confused it with freq since it takes 4 numbers...

OK i modified it to cover the both cases: numbers between 1-7 and enums MO, TH etc...
Looks like our rrule lib supports both.

cc @cnasikas Any feedback/comments on this schema?

I would advise following the schema as defined in x-pack/platform/plugins/shared/alerting/common/routes/r_rule/request/schemas/v1.ts as closely as possible. The byweekday can be a specific date like MO or -1WE, which can mean the last Wednesday of the month. In the UI this is configured like:

The rrule library can automatically handle this kind of input.

It would be nice if we had a schema in a package where you could import it (future work).

OK, I used the same schemas.

jloleysens

Overall looks good! However, I don't think we've quite concluded a related conversation. Would be good to do so before merging.

packages/kbn-check-mappings-update-cli/current_mappings.json

…ersin-erdal/kibana into 216308-add-rrule-to-task-model-versions

…te --fix'

… src/core/server/integration_tests/ci_checks'

ymao1 · 2025-04-29T22:40:53Z

packages/kbn-check-mappings-update-cli/current_mappings.json

          "interval": {
            "type": "keyword"
+          },
+          "rrule": {


probably don't need it at all, unless we're doing an exists query in the code somewhere currently?

removed all

…te --fix'

… src/core/server/integration_tests/ci_checks'

cnasikas · 2025-04-30T17:09:04Z

x-pack/platform/plugins/shared/task_manager/server/saved_objects/schemas/rrule.ts

+  bymonthday: schema.never(),
+});
+
+export const rruleSchedule = schema.oneOf([rruleMonthly, rruleWeekly, rruleDaily]);


What about having one schema and do feq: schema.oneOf[schema.literal(Frequency.DAILY), schema.literal(Frequency.WEEKLY), schema.literal(Frequency.MONTHLY].

I grouped them because some configurations should not be used with some frequency types, it looks the same for now but when we add hourly and minutely we will not allow byweekday and byhour together as they break the library.

…ersin-erdal/kibana into 216308-add-rrule-to-task-model-versions

jloleysens

Mappings changes LGTM, did not know you could add dynamic: false after creation like this, TIL

…ersin-erdal/kibana into 216308-add-rrule-to-task-model-versions

prodsecmachine · 2025-05-01T12:34:51Z

🎉 Snyk checks have passed. No issues have been found so far.

✅ security/snyk check is complete. No issues have been found. (View Details)

✅ license/snyk check is complete. No issues have been found. (View Details)

…te --fix'

… src/core/server/integration_tests/ci_checks'

ymao1

LGTM

ymao1 · 2025-05-01T14:58:53Z

@ersin-erdal I have your branch pulled into my scheduled report branch and am reusing these schemas for the scheduled report SO, I'm getting this type error for the rrule definition when it looks like this:

schedule: { rrule: { freq: Frequency.DAILY, interval: 2, tzid: 'UTC' } },

Type '{ freq: Frequency.DAILY; interval: number; tzid: string; }' is not assignable to type 'Mutable<Readonly<{ byhour?: number[] | undefined; byminute?: number[] | undefined; byweekday?: number[] | undefined; bymonthday?: number[] | undefined; } & { freq: Frequency.MONTHLY; interval: number; tzid: string; }> | Readonly<...> | Readonly<...>>'.
  Property 'bymonthday' is missing in type '{ freq: Frequency.DAILY; interval: number; tzid: string; }' but required in type 'Mutable<Readonly<{ byhour?: number[] | undefined; byminute?: number[] | undefined; byweekday?: number[] | undefined; } & { freq: Frequency.DAILY; interval: number; tzid: string; bymonthday: never; }>>'.
rrule.ts[Ln 52, Col 3]: 'bymonthday' is declared here.
task.ts[Ln 64, Col3]: The expected type comes from property 'rrule' which is declared here on type 'Mutable<Readonly<{} & { rrule: Readonly<{ byhour?: number[] | undefined; byminute?: number[] | undefined; byweekday?: number[] | undefined; bymonthday?: number[] | undefined; } & { freq: Frequency.MONTHLY; interval: number; tzid: string; }> | Readonly<...> | Readonly<...>; }>>'

which is confusing because bymonthday is schema.never but is it complaining that it's not defined? any ideas?

ymao1 · 2025-05-01T15:11:51Z

it works when i change it to schema.maybe(schema.never()) but I don't know if that makes sense

ersin-erdal · 2025-05-02T01:09:41Z

it works when i change it to schema.maybe(schema.never()) but I don't know if that makes sense

The only difference between the schemas here and in my other PR is { minSize: 1 } maybe that forces bymonthday to be defined even as undefined

I am adding schema.maybe to it, here as well. just in case.

…ersin-erdal/kibana into 216308-add-rrule-to-task-model-versions

ymao1 · 2025-05-02T02:31:01Z

I'm deriving the type from the schema using TypeOf...maybe there's a better way of doing that?

elasticmachine · 2025-05-02T13:51:06Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: b970cc2

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #1 / integrations Endpoint Exceptions "before all" hook for "should add event.module=endpoint to entry if only wildcard operator is present"

Metrics [docs]

✅ unchanged

History

💚 Build #298104 succeeded ae0e29d
💚 Build #297933 succeeded 8aea7eb
💚 Build #297802 succeeded 622cbc0
💛 Build #297402 was flaky 4478f57
💚 Build #297207 succeeded 6b2e3ef
💔 Build #297142 failed d24788c

kibanamachine · 2025-05-02T13:54:17Z

Starting backport for target branches: 8.19, 9.0

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

kibanamachine · 2025-05-02T14:00:54Z

💔 All backports failed

Status	Branch	Result
❌	8.19	Backport failed because of merge conflicts
❌	9.0	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 219429

Questions ?

Please refer to the Backport tool documentation

ersin-erdal · 2025-05-02T19:19:41Z

💚 All backports created successfully

Status	Branch	Result
✅	8.19

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…219429) Towards: elastic#216308 This PR adds the `rrule` schema to the task SO with an intermediate release for the following PR: elastic#217728 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 65cc416) # Conflicts: # src/core/server/integration_tests/ci_checks/saved_objects/check_registered_types.test.ts

…219429) (#219993) # Backport This will backport the following commits from `main` to `8.19`: - [Add rrule schema to task schedule with intermediate release (#219429)](#219429)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

…219429) Towards: elastic#216308 This PR adds the `rrule` schema to the task SO with an intermediate release for the following PR: elastic#217728 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

Add rrule schema to task schedule with intermediate release

2cf3878

ersin-erdal added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v9.0.0 backport:version Backport to applied version labels v8.19.0 v9.0.1 labels Apr 28, 2025

kibanamachine added 2 commits April 28, 2025 16:20

[CI] Auto-commit changed files from 'node scripts/check_mappings_upda…

dacbfc6

…te --fix'

[CI] Auto-commit changed files from 'node scripts/jest_integration -u…

c5069c7

… src/core/server/integration_tests/ci_checks'

ersin-erdal marked this pull request as ready for review April 28, 2025 20:05

ersin-erdal requested review from a team as code owners April 28, 2025 20:05

ersin-erdal mentioned this pull request Apr 28, 2025

Support rrule for task scheduling #217728

Merged

ymao1 reviewed Apr 29, 2025

View reviewed changes

jloleysens reviewed Apr 29, 2025

View reviewed changes

packages/kbn-check-mappings-update-cli/current_mappings.json Outdated Show resolved Hide resolved

ersin-erdal and others added 6 commits April 29, 2025 15:23

modify schema and remove mappings

38bffd6

Merge branch '216308-add-rrule-to-task-model-versions' of github.com:…

f846dbc

…ersin-erdal/kibana into 216308-add-rrule-to-task-model-versions

fix mappings_addition

d24788c

remove mappings from the packages

4d30c93

[CI] Auto-commit changed files from 'node scripts/check_mappings_upda…

6ca6280

…te --fix'

[CI] Auto-commit changed files from 'node scripts/jest_integration -u…

6b2e3ef

… src/core/server/integration_tests/ci_checks'

ymao1 reviewed Apr 29, 2025

View reviewed changes

ersin-erdal and others added 4 commits April 30, 2025 02:22

remove rrule mapping

a1b4481

fix interval mapping

be48567

[CI] Auto-commit changed files from 'node scripts/check_mappings_upda…

40c26ab

…te --fix'

[CI] Auto-commit changed files from 'node scripts/jest_integration -u…

4478f57

… src/core/server/integration_tests/ci_checks'

cnasikas reviewed Apr 30, 2025

View reviewed changes

ersin-erdal added 2 commits April 30, 2025 22:27

align rrule validation with the ones in alerting

c57f65e

Merge branch '216308-add-rrule-to-task-model-versions' of github.com:…

768160a

…ersin-erdal/kibana into 216308-add-rrule-to-task-model-versions

Merge branch 'main' into 216308-add-rrule-to-task-model-versions

622cbc0

jloleysens approved these changes May 1, 2025

View reviewed changes

ersin-erdal added 2 commits May 1, 2025 14:33

remove dynamic:false

66b6bfc

Merge branch '216308-add-rrule-to-task-model-versions' of github.com:…

39b9d76

…ersin-erdal/kibana into 216308-add-rrule-to-task-model-versions

kibanamachine added 2 commits May 1, 2025 12:49

[CI] Auto-commit changed files from 'node scripts/check_mappings_upda…

73f1df1

…te --fix'

[CI] Auto-commit changed files from 'node scripts/jest_integration -u…

8aea7eb

… src/core/server/integration_tests/ci_checks'

ymao1 approved these changes May 1, 2025

View reviewed changes

ersin-erdal added 2 commits May 2, 2025 03:10

add schema.maybe(

9563728

Merge branch '216308-add-rrule-to-task-model-versions' of github.com:…

ae0e29d

…ersin-erdal/kibana into 216308-add-rrule-to-task-model-versions

revert schema.maybe

b970cc2

ersin-erdal merged commit 65cc416 into elastic:main May 2, 2025
9 checks passed

kibanamachine added the v9.1.0 label May 2, 2025

kibanamachine mentioned this pull request May 2, 2025

[Incident Management] Investigation Guide backend #216377

Merged

ersin-erdal removed v9.0.0 v9.0.1 labels May 2, 2025

ersin-erdal mentioned this pull request May 2, 2025

[8.19] Add rrule schema to task schedule with intermediate release (#219429) #219993

Merged

kibanamachine mentioned this pull request May 2, 2025

Make interval in rrule config required #220003

Merged

Comments

Conversation

ersin-erdal commented Apr 28, 2025 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ersin-erdal Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnasikas Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

prodsecmachine commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎉 Snyk checks have passed. No issues have been found so far.

Uh oh!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

ymao1 commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ymao1 commented May 1, 2025

Uh oh!

ersin-erdal commented May 2, 2025

Uh oh!

ymao1 commented May 2, 2025

Uh oh!

elasticmachine commented May 2, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

Uh oh!

Uh oh!

kibanamachine commented May 2, 2025

Uh oh!

kibanamachine commented May 2, 2025

💔 All backports failed

Manual backport

Questions ?

Uh oh!

ersin-erdal commented May 2, 2025

💚 All backports created successfully

Questions ?

ersin-erdal commented Apr 28, 2025 •

edited by kibanamachine

Loading

ersin-erdal Apr 29, 2025 •

edited

Loading

cnasikas Apr 30, 2025 •

edited

Loading

prodsecmachine commented May 1, 2025 •

edited

Loading

ymao1 commented May 1, 2025 •

edited

Loading