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

[RAM][HTTP Versioning] Version Internal and Public Find Rules Route #180654

Merged
merged 10 commits into from
Apr 30, 2024

Conversation

JiaweiWu
Copy link
Contributor

Summary

Issue: #180551
Parent Issue: #157883

Versions the internal and public find API routes. Adds Input validation as well.

Checklist

@JiaweiWu JiaweiWu 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) Feature:Alerting/RulesManagement Issues related to the Rules Management UX v8.14.0 labels Apr 12, 2024
@JiaweiWu JiaweiWu requested review from Zacqary and umbopepato April 12, 2024 04:09
@JiaweiWu JiaweiWu requested a review from a team as a code owner April 12, 2024 04:09
@elasticmachine
Copy link
Contributor

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

@JiaweiWu
Copy link
Contributor Author

@elasticmachine merge upstream

@JiaweiWu
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@umbopepato umbopepato left a comment

Choose a reason for hiding this comment

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

Works as expected, LGTM!

image

@JiaweiWu JiaweiWu requested a review from a team as a code owner April 18, 2024 04:20
Copy link
Contributor

@maxcold maxcold left a comment

Choose a reason for hiding this comment

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

changes in cloud_security_posture lgtm

@cnasikas cnasikas requested review from cnasikas and removed request for Zacqary April 23, 2024 07:02
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! Tested and is working as expected.

),
filterConsumers: schema.maybe(schema.arrayOf(schema.string())),
},
{ unknowns: 'allow' }
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason of allowing unknowns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

security solution is passing through some props into this method that is passed through to the saved object client. These props are not listed as supported types to the find function. I dont think the props are doing anything but I was afraid to remove it.

Copy link
Member

Choose a reason for hiding this comment

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

These props are not listed as supported types to the find function

But they use it anyway? How TS does not throw an error? Did we support it in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it doesn't cause errors because we pull off all of the fields that the method uses. So extra props are just discarded. However we now validate the params so without passthrough, it starts to throw.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Thanks!

@JiaweiWu JiaweiWu linked an issue Apr 30, 2024 that may be closed by this pull request
@JiaweiWu JiaweiWu enabled auto-merge (squash) April 30, 2024 14:11
@JiaweiWu JiaweiWu added v8.15.0 and removed v8.14.0 labels Apr 30, 2024
@JiaweiWu JiaweiWu merged commit e6dcdc7 into elastic:main Apr 30, 2024
38 checks passed
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #64 / aiops "after all" hook in "aiops"
  • [job] [logs] FTR Configs #64 / aiops log rate analysis "after all" hook: afterTestSuite.trigger in "log rate analysis"
  • [job] [logs] FTR Configs #64 / aiops log rate analysis with 'artificial_logs_with_dip_textfield' "after all" hook for "artificial logs with dip and text field and no zero docs fallback displays index details"
  • [job] [logs] FTR Configs #64 / aiops log rate analysis with 'artificial_logs_with_dip_textfield' artificial logs with dip and text field and no zero docs fallback loads the log rate analysis page

Metrics [docs]

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
alerting 55 54 -1

History

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

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 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 Feature:Alerting/RulesManagement Issues related to the Rules Management UX 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.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RAM] Version Find Rule API
7 participants