-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[ML] Adding type for job summary state #131643
[ML] Adding type for job summary state #131643
Conversation
Pinging @elastic/ml-ui (:ml) |
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.
Code LGTM
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 ⚡
💚 Build SucceededMetrics [docs]Public APIs missing comments
Any counts in public APIs
Async chunks
Public APIs missing exports
History
To update your PR or re-run it, just comment with: |
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.
Security Solution changes LGTM 👍
The only nitpicky thing I noticed that could be improved: the type of jobState
could be adjusted in x-pack/plugins/security_solution/common/machine_learning/helpers.ts
as well:
// Based on ML Job/Datafeed States from x-pack/legacy/plugins/ml/common/constants/states.js
const enabledStates = ['started', 'opened'];
const loadingStates = ['starting', 'stopping', 'opening', 'closing'];
const failureStates = ['deleted', 'failed'];
export const isJobStarted = (jobState: string, datafeedState: string): boolean => {
return enabledStates.includes(jobState) && enabledStates.includes(datafeedState);
};
export const isJobLoading = (jobState: string, datafeedState: string): boolean => {
return loadingStates.includes(jobState) || loadingStates.includes(datafeedState);
};
export const isJobFailed = (jobState: string, datafeedState: string): boolean => {
return failureStates.includes(jobState) || failureStates.includes(datafeedState);
};
@banderror apologies, I hit merge before noticing this comment.
I think we need an audit of all plugins which are using our types and jobs to see how they are being used. The logs UI is in a similar situation where the code was written before we were doing a good job of exporting types.
* [ML] Adding type for job summary state * renaming type * translations * fixing jobState types in security * fixing snapshots * fixing test
Fixes a potential bug in Logs UI where jobs in a
deleting
,resetting
andreverting
state will cause an error.Closes #130258
Also removes the translation of the
jobState
valuesdeleting
,resetting
andreverting
. These were being incorrectly combined with the job state from es, which is not translated.So now
deleting
,resetting
andreverting
will always appear in english in the UI along with other job states, likeclosed
opening
etc