[ML] New Platform server shim: update job validation routes #57644
Conversation
alvarezmelissa87
added
:ml
v8.0.0
release_note:skip
|
Pinging @elastic/ml-ui (:ml) |
alvarezmelissa87
changed the title
There was a problem hiding this comment.
NIT: Should use APICaller interface instead (same in other .d.ts file)
import { APICaller } from 'src/core/server';
export function estimateBucketSpanFactory(
callAsCurrentUser: APICaller,
callAsInternalUser: APICaller,There was a problem hiding this comment.
Updated in f0bac33e423d5a570bfa2ba8157a9d1e677c6afb
There was a problem hiding this comment.
Is it normal that callWithRequest was renamed to callAsInternalUser? shouldn't it be callAsCurrentUser instead? Or did the behavior change? (same question on some other files)
There was a problem hiding this comment.
Yep, you're right - I was thinking callAsInternalUser was getting passed in but it is callAsCurrentUser. I'll update it. Good catch 👌
Fixed in f0bac33e423d5a570bfa2ba8157a9d1e677c6afb
There was a problem hiding this comment.
NIT: kibana/server and src/core/server are the same alias. Use only one import line.
There was a problem hiding this comment.
updated in f0bac33e423d5a570bfa2ba8157a9d1e677c6afb
There was a problem hiding this comment.
Question: don't know the functional domain, but schema.arrayOf(schema.nullable(schema.string())) looks strange to me. can/should it really accept something like ['foo', null, 'bar', null] ?
There was a problem hiding this comment.
Yep - so this was based on the BucketEstimatorData type interface (https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/ml/public/application/services/ml_api_service/index.d.ts#L48) used to define the type of data sent to the bucket span estimator endpoint.
There was a problem hiding this comment.
(Already present before the PR, but) is there any reason the anomalyDetectionJobSchema is not already wrapped with a schema.object()? I see that some are and some aren't in x-pack/legacy/plugins/ml/server/new_platform/anomaly_detectors_schema.ts
There was a problem hiding this comment.
NIT: the handler calling that seems to have a proper body validation schema. Could probably get rid of the any signature here
type youWillNameThatBetterThanMePayload = TypeOf<typeof schema.object(modelMemoryLimitSchema)>There was a problem hiding this comment.
This I did not know about and it is beautiful. I thank you 🙏
Updated in f0bac33e423d5a570bfa2ba8157a9d1e677c6afb
alvarezmelissa87
force-pushed
the
ml-new-platform-job-validation-routes
branch
from
4e3fb2f to
728fa12
Compare
alvarezmelissa87
changed the title
alvarezmelissa87
requested review from
darnautov,
jgowdyelastic and
walterra
alvarezmelissa87
force-pushed
the
ml-new-platform-job-validation-routes
branch
from
c60bb3c to
6814dd8
Compare
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…57644) * wip: convert jobValidation routes to NP * fix typo in np migration docs * replace legacy es plugin with callAsInternalUser from context * add schema info for job validation routes * update types and schema for job + cardinality validation * update bucketSpanEstimator test * update job validation schema
…57874) * wip: convert jobValidation routes to NP * fix typo in np migration docs * replace legacy es plugin with callAsInternalUser from context * add schema info for job validation routes * update types and schema for job + cardinality validation * update bucketSpanEstimator test * update job validation schema
6 participants
Summary
Related meta issue: #49743
Updates all job validation routes to use new platform router and replaces use of legacy
elasticsearchPluginincallWithInternalUserFactorywith NP'scontext.core.elasticsearch.adminClient.callAsInternalUserAlso fixes type in migration doc.
Checklist
Delete any items that are not applicable to this PR.