Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Adds support for Kibana 7.7.0 #151

Merged
merged 5 commits into from
May 29, 2020
Merged

Adds support for Kibana 7.7.0 #151

merged 5 commits into from
May 29, 2020

Conversation

ftianli-amzn
Copy link

@ftianli-amzn ftianli-amzn commented May 19, 2020

Issue #, if available:
#150

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ftianli-amzn
Copy link
Author

Test Suites: 6 skipped, 78 passed, 78 of 84 total
Tests:       20 skipped, 366 passed, 386 total
Snapshots:   4 updated, 123 passed, 127 total
Time:        26.488s
Ran all test suites.
✨  Done in 27.45s.

@dbbaughe
Copy link
Contributor

Did you run into any Kibana icon styling issue? I believe other teams were running in to that.

@ftianli-amzn
Copy link
Author

ftianli-amzn commented May 20, 2020

Did you run into any Kibana icon styling issue? I believe other teams were running in to that.

😳 I haven't started a Kibana server to test yet, sorry.
Update: The issue is there.

ylwu-amzn
ylwu-amzn previously approved these changes May 20, 2020
Copy link
Contributor

@ylwu-amzn ylwu-amzn left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ohltyler ohltyler left a comment

Choose a reason for hiding this comment

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

Found an issue on ODFE test domain. When creating a monitor from a detector, I've ran into an exception causing a blank page. Seems to be related to another deprecated getter that needs to be removed: getAnnotationId(). In console:

Screen Shot 2020-05-26 at 10 14 30 AM

@ftianli-amzn
Copy link
Author

ftianli-amzn commented May 27, 2020

Found an issue on ODFE test domain. When creating a monitor from a detector, I've ran into an exception causing a blank page. Seems to be related to another deprecated getter that needs to be removed: getAnnotationId(). In console:

Thank you so much for catching this hidden issue! 👏

@ftianli-amzn
Copy link
Author

Test Suites: 6 skipped, 78 passed, 78 of 84 total
Tests: 20 skipped, 366 passed, 386 total
Snapshots: 127 passed, 127 total
Time: 50.188s
Ran all test suites.
✨ Done in 52.61s.

@ohltyler
Copy link
Contributor

@ftianli-amzn tested on latest Ubuntu image (created ~6 hours ago), error is still there. Did you push new artifacts last night?

@ftianli-amzn
Copy link
Author

@ftianli-amzn tested on latest Ubuntu image (created ~6 hours ago), error is still there. Did you push new artifacts last night?

Ah, sorry I didn't push the artifacts yet..

@ohltyler
Copy link
Contributor

@ftianli-amzn tested on latest Ubuntu image (created ~6 hours ago), error is still there. Did you push new artifacts last night?

Ah, sorry I didn't push the artifacts yet..

no problem. will check tomorrow

@ftianli-amzn
Copy link
Author

@ftianli-amzn tested on latest Ubuntu image (created ~6 hours ago), error is still there. Did you push new artifacts last night?

Ah, sorry I didn't push the artifacts yet..

no problem. will check tomorrow

Have uploaded, hope there will be a new docker image created earlier.

dbbaughe
dbbaughe previously approved these changes May 27, 2020
@ohltyler
Copy link
Contributor

Still some exceptions being thrown. Looks like getAxisId still exists. I'd double check that there are no occurrences of any of these functions in the entire codebase.

Screen Shot 2020-05-28 at 8 35 50 AM

@ftianli-amzn
Copy link
Author

Still some exceptions being thrown. Looks like getAxisId still exists. I'd double check that there are no occurrences of any of these functions in the entire codebase.

Thank you for testing the plugin, Tyler! I noticed there were remaining getAxisId, getSpecId in FeatureCharts.js. I have double checked there are no such deprecated getter functions any more. 😒

@dbbaughe
Copy link
Contributor

@ylwu-amzn Were there tests written for this component? Wouldn't a simple snapshot test catch this not being able to render?

@ftianli-amzn
Copy link
Author

ftianli-amzn commented May 28, 2020

I will do a manually test for this feature today in case there are other exceptions thrown.
Update: Have tested, and no error occurs when creating a monitor whose "Method of definition" is anomaly detector.

@ftianli-amzn
Copy link
Author

Thank you all for taking look at this PR, I will merge it shortly.

@ylwu-amzn
Copy link
Contributor

@ylwu-amzn Were there tests written for this component? Wouldn't a simple snapshot test catch this not being able to render?

Good suggestion. We should add more test cased for AD components. Created an issue #156.

@ftianli-amzn
Copy link
Author

@ylwu-amzn Were there tests written for this component? Wouldn't a simple snapshot test catch this not being able to render?

Good suggestion. We should add more test cased for AD components. Created an issue #156.

Thank you for opening an issue! Nice

@ftianli-amzn ftianli-amzn merged commit ea63b55 into opendistro-for-elasticsearch:master May 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants