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

[ResponseOps][Rules] Version unmute alert route #188830

Merged
merged 6 commits into from
Jul 30, 2024

Conversation

umbopepato
Copy link
Member

@umbopepato umbopepato commented Jul 22, 2024

Summary

Versions the POST /api/alerting/rule/{rule_id}/alert/{alert_id}/_unmute enpoint.

References

Parent issue: #187572
Closes #187574

Checklist

@umbopepato umbopepato 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) v8.16.0 labels Jul 22, 2024
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato force-pushed the 187572-version-unmute-alert-route branch from 5150716 to 47f6edd Compare July 22, 2024 12:59
@umbopepato umbopepato requested review from JiaweiWu, ersin-erdal and a team and removed request for a team July 22, 2024 12:59
@umbopepato umbopepato marked this pull request as ready for review July 22, 2024 13:01
@umbopepato umbopepato requested a review from a team as a code owner July 22, 2024 13:01
@elasticmachine
Copy link
Contributor

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

@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato force-pushed the 187572-version-unmute-alert-route branch from 47f6edd to 552505c Compare July 22, 2024 13:05
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! I will the final approval to @JiaweiWu.

Copy link
Contributor

@adcoelho adcoelho left a 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> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be <UnmuteAlertParamsV1> ?

Copy link
Member Author

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? 🤔)

Copy link
Member

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?

@umbopepato
Copy link
Member Author

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.

Good catch, thanks! 👍

@umbopepato umbopepato force-pushed the 187572-version-unmute-alert-route branch from 2671c69 to 9c2ee86 Compare July 29, 2024 13:51
@umbopepato umbopepato force-pushed the 187572-version-unmute-alert-route branch from 9c2ee86 to d9aa442 Compare July 30, 2024 13:19
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #3 / ColumnsPopover renders correctly a list of selected columns

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@umbopepato umbopepato merged commit d88a1b4 into elastic:main Jul 30, 2024
43 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ResponseOps][Rules] OAS schema registration for Rule APIs
6 participants