-
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] Transforms: Adds reset action to transforms management. #123735
Conversation
Pinging @elastic/ml-ui (:ml) |
if (results.hasOwnProperty(transformId)) { | ||
const status = results[transformId]; | ||
|
||
// if we are only deleting one transform, show the success toast messages |
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.
super duper nit: think the deleting
in the comment here and below meant to be resetting
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.
Fixed in 044b4da.
): arg is ResetTransformsResponseSchema => { | ||
return ( | ||
isPopulatedObject(arg) && | ||
Object.values(arg).every((d) => isPopulatedObject(d, ['transformReset'])) |
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.
Will there ever be a case a response has a mix of transformReset and something else? Can we use .some
here? Just curious not a suggestion.
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.
The current APIs we have don't allow these combinations at the moment. Depending on the endpoint, this attribute is hard coded for example like transformDeleted
or transformReset
.
Tested (normal and managed transforms) and functionally LGTM 🎉 . Just had a nit comment. |
const bulkResetButtonDisabledText = i18n.translate( | ||
'xpack.transform.transformList.resetBulkActionDisabledToolTipContent', | ||
{ | ||
defaultMessage: 'One or more selected transforms must be stopped in order to be reseted.', |
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.
Typo reseted
. Maybe One or more selected transforms must be stopped in order to be reset
?
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.
Maybe we can make it a bit shorter and it might also sound better.
defaultMessage: 'One or more selected transforms must be stopped in order to be reseted.', | |
defaultMessage: 'One or more selected transforms must be stopped to be reset.', |
disabled, | ||
isBulkAction, | ||
}) => { | ||
const bulkResetButtonDisabledText = i18n.translate( |
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.
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.
Good catch, I will look into this in a follow up since it's not only affecting the new reset functionality.
const bulkResetButtonDisabledText = i18n.translate( | ||
'xpack.transform.transformList.resetBulkActionDisabledToolTipContent', | ||
{ | ||
defaultMessage: 'One or more selected transforms must be stopped in order to be reseted.', |
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.
defaultMessage: 'One or more selected transforms must be stopped in order to be reseted.', | |
defaultMessage: 'Stop one or more selected transforms to reset them.', |
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.
Fixed as part of ab14042.
@elasticmachine merge upstream |
expected head sha didn’t match current head ref. |
💛 Build succeeded, but was flakyTest Failures
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @walterra |
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.
Latest code changes LGTM
Summary
Fixes #118405.
Adds reset action to transforms management. Supports bulk resetting multiple transforms.
Checklist