-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ResponseOps][Rules] Version disable rule route #188659
Conversation
Pinging @elastic/response-ops (Team:ResponseOps) |
💚 Build Succeeded
Metrics [docs]
History
cc @adcoelho |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -585,6 +585,16 @@ describe('disable()', () => { | |||
expect(taskManager.removeIfExists).not.toHaveBeenCalledWith(); | |||
}); | |||
|
|||
test('throws if API params do not match the schema', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: should we also validate the untrack
param?
|
||
import { schema } from '@kbn/config-schema'; | ||
|
||
export const disableRuleRequestBodySchema = schema.nullable( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: we can give this entire body schema a default value so we wouldn't have to check for null in the route level
const { id } = req.params; | ||
const { untrack = false } = req.body || {}; | ||
const { id }: DisableRuleRequestParamsV1 = req.params; | ||
const body: DisableRuleRequestBodyV1 = req.body || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above is referring to this
try { | ||
await rulesClient.disable({ id, untrack }); | ||
await rulesClient.disable(disableParams); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's rename this to disableRule
, so disable.ts
-> disable_rule.ts
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @adcoelho |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
Parent Issue: #187572
Versions the
POST /rule/{id}/_disable
endpoint.Checklist