Add rule model versions in alerting #171927
Conversation
ersin-erdal
added
release_note:skip
There was a problem hiding this comment.
As this is used by Kibana to validate the saved_object just before saving it, we can remove the below schema from createRule method of the rulesClient.
… src/core/server/integration_tests/ci_checks'
…67-model-versions
|
Pinging @elastic/response-ops (Team:ResponseOps) |
| @@ -209,6 +210,7 @@ const rawRuleActionSchema = schema.object({ | |||
| }) | |||
| ), | |||
| alertsFilter: schema.maybe(rawRuleAlertsFilterSchema), | |||
| useAlertDataForTemplate: schema.maybe(schema.boolean()), | |||
There was a problem hiding this comment.
This is/was a new field and causes a validation error.
There was a problem hiding this comment.
I don't understand the purpose of field and useAlertDataForTemplate. Is for use in the future? It seems like it could have been a test you were running locally, didn't intend to commit, but you added a comment, so seems not.
There was a problem hiding this comment.
Someone added that new field and forgot to update the rawRuleSchema, therefore createRule method fails.
I added it to fix the problem.
There was a problem hiding this comment.
I don't know how they missed that, task execution would be skipped when that field is in any rule.
There was a problem hiding this comment.
Interesting! I wonder if it coincided with some other "leniency" PRs where we might have been more lenient in accepting some objects validating schemas (allowing extra fields, but ignoring).
I was going to suggest opening an issue to figure out how this happened, because it doesn't seem good. However, we ARE now catching it :-), so ... not sure it matters. Presumably we'd catch the next time this happens.
There was a problem hiding this comment.
Actually that's why we decided to use modelVersions. It will force developers to bump the version.
No overlooked new fields anymore :)
|
Are there any specific steps to verify this PR or things to look for? |
As the modelVersions schema is called on |
|
closed by mistake |
|
This is the PR that added it: #170162 |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @ersin-erdal |
| @@ -266,5 +268,6 @@ export const rawRuleSchema = schema.object({ | |||
| severity: schema.maybe(schema.string()), | |||
| }) | |||
| ), | |||
| params: schema.recordOf(schema.string(), schema.any()), | |||
| params: schema.recordOf(schema.string(), schema.maybe(schema.any())), | |||
There was a problem hiding this comment.
I'm curious why we needed the schema.maybe() - I don't think it needs to be, unless someone is referencing fields in pararms and the typing was messed up. And I don't see any references to the field in the PR ... But I don't think there's any harm in this either. I'm more curious than concerned. :-)
Though there is a different field params in this file ^^^ (above the new field property) which uses the same schema bits, but differently:
params: schema.maybe(schema.recordOf(schema.string(), schema.any())), // better type?I just tried the following, and it only failed on the last one - I wasn't sure about the second, but it appears to have validated .
it('tests schema.any()', () => {
const testSchema = schema.object({
params: schema.recordOf(schema.string(), schema.maybe(schema.any())),
});
testSchema.validate({ params: { test: 'test' } });
testSchema.validate({ params: { test: undefined } });
testSchema.validate({ params: {} });
testSchema.validate({});
});There was a problem hiding this comment.
In one of the test a team uses the second. { params: { test: undefined } } and it blows up schema.any() :)
There was a problem hiding this comment.
My "test" was using your code, I wanted to remove the schema.maybe() to see if the second would pass - but I believe you! :-) . Thx!
Towards: #166967
This PR adds
ruleModelVersionto saved object registry andlatestRuleVersionto ruleTypeRegistry.These new assets will be used in a follow-on PR for skipping the rule task executions when there is version mismatch.
POC for the issue: #167128