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.

Sign up for GitHub

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

Jump to bottom

[ML] Display warning if clone job fails due to missing data view #117993

Merged
merged 3 commits into from
Nov 11, 2021
Merged

[ML] Display warning if clone job fails due to missing data view #117993

merged 3 commits into from
Nov 11, 2021

Conversation

peteharverson
Copy link
Contributor

Summary

Fixes regression introduced by #116455, adding back the warning toast if cloning an anomaly detection job fails because there is no data view matching the indices used in the datafeed.

Also adds a new functional test which would have caught the original regression.

image

Checklist

Fixes #117834

@peteharverson peteharverson added review :ml Feature:Anomaly Detection ML anomaly detection v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v8.1.0 labels Nov 9, 2021
@peteharverson peteharverson self-assigned this Nov 9, 2021
@peteharverson peteharverson requested a review from a team as a code owner November 9, 2021 10:56
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

darnautov
darnautov reviewed Nov 9, 2021
View reviewed changes
x-pack/plugins/ml/public/application/jobs/jobs_list/components/utils.js Outdated
values: { jobId, dataViewTitle },
}
);
getToastNotifications().addDanger({
Copy link
Contributor

Choose a reason for hiding this comment

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

should we use getToastNotificationService instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched this one to getToastNotificationService in b67a413.

x-pack/test/functional/services/ml/job_table.ts Outdated
public async assertNoDataViewForCloneJobWarningToastExist() {
await retry.tryForTime(5000, async () => {
const toast = await testSubjects.find('mlCloneJobNoDataViewExistsWarningToast');
expect(toast).not.to.be(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to check the content of the toast?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's ok just to check the data-test-subj for now, and this is in line with other tests we have for toast notifications.

darnautov
darnautov approved these changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM, I think we can replace getToastNotifications with getToastNotificationService in a separate PR for the entire file

@peteharverson peteharverson mentioned this pull request Nov 9, 2021
Closed
3 tasks
pheyos
pheyos reviewed Nov 9, 2021
View reviewed changes
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Tested and the fix worked fine.
Just two comments / suggestions for the test.

x-pack/test/functional/services/ml/job_table.ts Outdated Show resolved Hide resolved
x-pack/test/functional/apps/ml/anomaly_detection/single_metric_job.ts Outdated Show resolved Hide resolved
alvarezmelissa87
alvarezmelissa87 approved these changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

LGTM ⚡

pheyos
pheyos approved these changes Nov 11, 2021
View reviewed changes
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

LGTM

@peteharverson
Copy link
Contributor Author

@elasticmachine merge upstream

@peteharverson peteharverson added the auto-backport Deprecated - use backport:version if exact versions are needed label Nov 11, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 3.5MB 3.5MB +345.0B

History

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

cc @peteharverson

@peteharverson peteharverson merged commit f440b3b into elastic:main Nov 11, 2021
@peteharverson peteharverson deleted the ml-clone-job-no-data-view-toast branch November 11, 2021 15:39
@kibanamachine kibanamachine mentioned this pull request Nov 11, 2021
Merged
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 11, 2021
@peteharverson @kibanamachine
[ML] Display warning if clone job fails due to missing data view (ela…
0fd2287 
…stic#117993)

* [ML] Display warning if clone job fails due to missing data view

* [ML] Edits following review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 8.0

This backport PR will be merged automatically after passing CI.

claudiopro pushed a commit to claudiopro/kibana that referenced this pull request Nov 11, 2021
@peteharverson @kibanamachine @claudiopro
[ML] Display warning if clone job fails due to missing data view (ela…
5f335a5 
…stic#117993)

* [ML] Display warning if clone job fails due to missing data view

* [ML] Edits following review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kibanamachine added a commit that referenced this pull request Nov 11, 2021
@kibanamachine @peteharverson
[ML] Display warning if clone job fails due to missing data view (#11…
c61386b 
…7993) (#118373)

* [ML] Display warning if clone job fails due to missing data view

* [ML] Edits following review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Pete Harverson <pete@elastic.co>
fkanout pushed a commit to fkanout/kibana that referenced this pull request Nov 17, 2021
@peteharverson @kibanamachine @fkanout
[ML] Display warning if clone job fails due to missing data view (ela…
7d49c73 
…stic#117993)

* [ML] Display warning if clone job fails due to missing data view

* [ML] Edits following review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021
@peteharverson @kibanamachine @dmlemeshko
[ML] Display warning if clone job fails due to missing data view (#11…
2d8263b 
…7993)

* [ML] Display warning if clone job fails due to missing data view

* [ML] Edits following review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
roeehub pushed a commit to build-security/kibana that referenced this pull request Dec 16, 2021
@peteharverson @kibanamachine
[ML] Display warning if clone job fails due to missing data view (ela…
6e57442 
…stic#117993)

* [ML] Display warning if clone job fails due to missing data view

* [ML] Edits following review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pheyos pheyos pheyos approved these changes

@darnautov darnautov darnautov approved these changes

@alvarezmelissa87 alvarezmelissa87 alvarezmelissa87 approved these changes

Assignees

@peteharverson peteharverson

Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Anomaly Detection ML anomaly detection :ml release_note:skip Skip the PR/issue when compiling release notes review v8.0.0 v8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ML] No warning shown if attempting to clone a job for which there is no data view
6 participants
@peteharverson @elasticmachine @kibanamachine @pheyos @darnautov @alvarezmelissa87