-
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 unmute alert route #188830
[ResponseOps][Rules] Version unmute alert route #188830
Conversation
/ci |
5150716
to
47f6edd
Compare
Pinging @elastic/response-ops (Team:ResponseOps) |
/ci |
47f6edd
to
552505c
Compare
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! I will the final approval to @JiaweiWu.
...erver/routes/rule/apis/unmute_alert/transforms/transform_request_params_to_application/v1.ts
Show resolved
Hide resolved
552505c
to
f9ca6d5
Compare
x-pack/plugins/alerting/server/routes/rule/apis/unmute_alert/unmute_alert.ts
Outdated
Show resolved
Hide resolved
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.
Could you please rename
x-pack/plugins/alerting/server/routes/rule/apis/unmute_alert/unmute_alert.ts
and
x-pack/plugins/alerting/server/routes/rule/apis/unmute_alert/unmute_alert.test.ts
to unmute_alert_route.ts
and unmute_alert_route.test.ts
for consistency?
And a small nit, you can also add #187574 in the description, this PR will close both issues I think.
import { UnmuteAlertParams } from '../../../../../../application/rule/methods/unmute_alert/types'; | ||
import { RewriteRequestCase } from '../../../../../lib'; | ||
|
||
export const transformRequestParamsToApplication: RewriteRequestCase<UnmuteAlertParams> = ({ |
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.
Should this be <UnmuteAlertParamsV1>
?
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.
Ah good point. I followed the same structure of the mute_alert
endpoint, where that type is not versioned. It seems like none of the types in alerting/server/application/rule/methods is versioned (guess it makes sense since they're used as parameter types of the rules client methods, which have no multiple coexisting versions? 🤔)
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 rule methods are not versioned intentionally. The transformation functions are versioned. The reason is that they transform a versioned request to a non-versioned method (application) schema. So we transform from UnmuteAlertRequestParamsV1
(from routes/rule/apis/unmute_alert/index.ts
) -> UnmuteAlertParams
(from application/rule/methods/unmute_alert/types
). I would suggest to be more explicit for the types of the params like
export const transformRequestParamsToApplication: RewriteRequestCase<UnmuteAlertParams> = ({rule_id: alertId,
alert_id: alertInstanceId,
}: UnmuteAlertRequestParamsV1) => ({
alertId,
alertInstanceId,
});
Wdyt?
Good catch, thanks! 👍 |
2671c69
to
9c2ee86
Compare
Co-authored-by: Antonio <antoniodcoelho@gmail.com>
9c2ee86
to
d9aa442
Compare
💛 Build succeeded, but was flaky
Failed CI StepsTest FailuresMetrics [docs]
History
To update your PR or re-run it, just comment with: |
Summary
Versions the
POST /api/alerting/rule/{rule_id}/alert/{alert_id}/_unmute
enpoint.References
Parent issue: #187572
Closes #187574
Checklist