[Enterprise Search] Added App Search log settings routes #82162
Conversation
| async (context, request, response) => { | ||
| return enterpriseSearchRequestHandler.createRequest({ | ||
| path: '/as/log_settings', | ||
| })(context, request, response); | ||
| } |
There was a problem hiding this comment.
Not a change request, just me wondering - do we want to simplify this to
| async (context, request, response) => { | |
| return enterpriseSearchRequestHandler.createRequest({ | |
| path: '/as/log_settings', | |
| })(context, request, response); | |
| } | |
| enterpriseSearchRequestHandler.createRequest({ | |
| path: '/as/log_settings', | |
| }) |
at all? Or do we like the more explicit curried function notation that we end up having to use on some more complex API endpoints?
There was a problem hiding this comment.
Yeah think we should. It simplifies the code and tests a bit.
|
|
||
| import { IRouteDependencies } from '../../plugin'; | ||
|
|
||
| const logSettingsSchema = schema.object({ |
There was a problem hiding this comment.
Do we anticipate multiple POST/PUT endpoints using this schema in the future? In the credentials router file, I pulled a var out to the top primarily because it was super complex & reused between 2 different endpoints - in this case we could just leave this explicitly on line 44.
But also no objections/no strong feelings if we like leaving it at the top of files so it's the first thing people see 🤷
| @@ -41,6 +41,7 @@ import { appSearchTelemetryType } from './saved_objects/app_search/telemetry'; | |||
| import { registerTelemetryUsageCollector as registerASTelemetryUsageCollector } from './collectors/app_search/telemetry'; | |||
| import { registerEnginesRoute } from './routes/app_search/engines'; | |||
| import { registerCredentialsRoutes } from './routes/app_search/credentials'; | |||
| import { registerLogSettingsRoutes } from './routes/app_search/log_settings'; | |||
There was a problem hiding this comment.
Just curious - do we need to specifically call this out as log_settings vs. just settings? I think I have a slight preference to simplifying this to just
| import { registerLogSettingsRoutes } from './routes/app_search/log_settings'; | |
| import { registerSettingsRoutes } from './routes/app_search/settings'; |
I think that's fine because on the off-chance we add more non-ILM/log settings in the future that call different API endpoints, we can just continue to add more router APIs to the same file, and it's not a huge deal because they conceptually fall under the same "Settings" bucket.
There was a problem hiding this comment.
@constancecchen Yeah I think that's a good suggestion, I'll change this file to be "settings"
| import { registerLogSettingsRoutes } from './log_settings'; | ||
|
|
||
| describe('log settings routes', () => { | ||
| describe('PUT /api/app_search/log_settings', () => { |
There was a problem hiding this comment.
I don't have super strong feelings and this is not a blocker, but I might suggest generally putting ordering our GET endpoint to follow both user flow and the source code, and the PUT tests after
There was a problem hiding this comment.
Good suggestion, I'll do that
💚 Build SucceededMetrics [docs]distributable file count
History
To update your PR or re-run it, just comment with: |
JasonStoltz
added
release_note:skip
Summary
This PR adds server routes for App Search's log settings
Checklist
Delete any items that are not applicable to this PR.
For maintainers