[ML] New Platform server shim: update calendar routes to use new platform router #56264

alvarezmelissa87 · 2020-01-29T00:46:58Z

Summary

Related meta issue: #49743

Updates all calendar routes to use new platform router.

Adds an isLegacy parameter to the CalendarManager so that it's still compatible with other route creators using callWithRequest.

I've left the type of client passed in as any for now, as job routes will be updated very soon and we'll be able to remove that legacy parameter and remove the need to have both types in CalendarManager.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~
~~- [ ] Unit or functional tests were updated or added to match the most common scenarios~~
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2020-01-29T00:47:01Z

Pinging @elastic/ml-ui (:ml)

jgowdyelastic · 2020-01-29T08:41:57Z

x-pack/legacy/plugins/ml/server/models/calendar/calendar_manager.ts

+
+  constructor(isLegacy: boolean, client: any) {
+    this.isLegacy = isLegacy;
+    this.client = client;


it might be worth making the isLegacy check once here in the constructor and setting this.client as either client or client.ml!.mlClient.callAsCurrentUser
Or even making the check outside of this class and only passing in the correct client when instantiating the class

Good call- much better. Added the legacy check in the constructor here: e4c0e64639120c16c78f85258334091d4ec0c6ea

jgowdyelastic · 2020-01-29T08:46:07Z

x-pack/legacy/plugins/ml/server/models/calendar/calendar_manager.ts

    try {
-      const resp = await this.callWithRequest('ml.calendars', { calendarId });
+      let resp;
+      if (this.isLegacy === true) {


as per the previous comment, having these checks inside the class seems unnecessary and means we'll have to change them again at some point in the future.

I've just seen the comment on this PR referring to the need for compatibility with the still legacy job service, so please ignore the suggestion of moving these checks out side of the class.
I still think the check could be moved to the constructor, so it's only happening once also isLegacy wouldn't need to be stored.

jgowdyelastic · 2020-01-29T08:48:36Z

x-pack/legacy/plugins/ml/server/routes/calendars.ts

+      try {
+        const calendarIds = request.params.calendarIds.split(',');
+
+        if (calendarIds.length === 1) {


nit, this could be a ternary

jgowdyelastic · 2020-01-29T09:25:56Z

x-pack/legacy/plugins/ml/server/models/calendar/calendar_manager.ts

-    this.callWithRequest = callWithRequest;
-    this.eventManager = new EventManager(callWithRequest);
+  private isLegacy: boolean;
+  private client: any;


for recent classes that i've written i've started all private class members with an underscore.
I don't know if this is something we want to try to do everywhere. I quite like the naming convention as it makes it easy to spot private and public members.
Other parts of Kibana have adopted this pattern but it seems to depend on the developer.

Updated naming these with the underscore - e4c0e64639120c16c78f85258334091d4ec0c6ea

jgowdyelastic · 2020-01-29T09:28:16Z

x-pack/legacy/plugins/ml/server/models/calendar/calendar_manager.ts

@@ -6,17 +6,46 @@

 import _ from 'lodash';


it looks like only difference is used from lodash

Updated to only pull in difference e4c0e64639120c16c78f85258334091d4ec0c6ea

jgowdyelastic · 2020-01-29T09:30:46Z

x-pack/legacy/plugins/ml/server/models/calendar/event_manager.ts

@@ -0,0 +1,94 @@
+/*


it's strange that calenadar_manager has been marked as a js -> ts rename by git, but this file hasn't.
it would be useful to try to retain the git history

yeah, I'm not sure why that's the case. I'll look into it.

it looks ok now. it shows it as a moved file. one of your subsequent comments must have made the difference.
probably removing the churn by removing all the isLegacy checks

peteharverson

LGTM.

Just one question - I guess the ts-ignore can be removed from the routes that have been converted to TS. This calendars one, plus the one for data_frame_analytics which has already been done?

jgowdyelastic

LGTM

alvarezmelissa87 · 2020-01-29T21:19:25Z

Functional test failures were being caused by a missing definition in the schema (in the route validation) for calendar_id (as both that and the camel case version are used sometimes) and for event_id. This was causing the endpoint to return an error. I've updated the schema to include both of those as optional and tested that it works. 0d9e425

Will address removing use of both calendar_id and calendarId in a follow up.

cc @peteharverson, @jgowdyelastic

kibanamachine · 2020-01-29T22:37:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0d9e425

History

💔 Build #23359 failed 9908a66
💔 Build #23331 failed 95785c207669fcc47ad701ca6f16e81c134bcf2d
💔 Build #23303 failed e4c0e64639120c16c78f85258334091d4ec0c6ea
💔 Build #23214 failed 637dab2a108aac4d0e0558c40965bba946789864

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

jgowdyelastic · 2020-01-30T07:47:24Z

Tested and calendar editing issue is fixed. LGTM

peteharverson

Latest edit LGTM

…form router (elastic#56264) * migrate calendar routes to NP * add proper types for calendars and events * set actual client in constructor so isLegacy is not stored * remove unnecessary comments * fix calendar schema for missing calendar_id and event_id properties

…form router (#56264) (#56379) * migrate calendar routes to NP * add proper types for calendars and events * set actual client in constructor so isLegacy is not stored * remove unnecessary comments * fix calendar schema for missing calendar_id and event_id properties

alvarezmelissa87 added :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.7.0 labels Jan 29, 2020

alvarezmelissa87 requested review from darnautov, peteharverson and jgowdyelastic January 29, 2020 00:46

alvarezmelissa87 requested a review from a team as a code owner January 29, 2020 00:46

alvarezmelissa87 self-assigned this Jan 29, 2020

peteharverson mentioned this pull request Jan 29, 2020

[ML] New platform migration #49743

Closed

78 tasks

jgowdyelastic reviewed Jan 29, 2020

View reviewed changes

peteharverson approved these changes Jan 29, 2020

View reviewed changes

jgowdyelastic approved these changes Jan 29, 2020

View reviewed changes

alvarezmelissa87 added 4 commits January 29, 2020 13:07

migrate calendar routes to NP

3dcdda0

add proper types for calendars and events

fc1f931

set actual client in constructor so isLegacy is not stored

dec8f64

remove unnecessary comments

9908a66

alvarezmelissa87 force-pushed the ml-new-platform-calendar-routes branch from 95785c2 to 9908a66 Compare January 29, 2020 18:07

fix calendar schema for missing calendar_id and event_id properties

0d9e425

peteharverson approved these changes Jan 30, 2020

View reviewed changes

alvarezmelissa87 merged commit fb334ef into elastic:master Jan 30, 2020

alvarezmelissa87 mentioned this pull request Jan 30, 2020

[7.x] [ML] New Platform server shim: update calendar routes to use new platform router (#56264) #56379

Merged

alvarezmelissa87 deleted the ml-new-platform-calendar-routes branch January 30, 2020 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] New Platform server shim: update calendar routes to use new platform router #56264

[ML] New Platform server shim: update calendar routes to use new platform router #56264

alvarezmelissa87 commented Jan 29, 2020 •

edited

Loading

elasticmachine commented Jan 29, 2020

jgowdyelastic Jan 29, 2020 •

edited

Loading

alvarezmelissa87 Jan 29, 2020

jgowdyelastic Jan 29, 2020

jgowdyelastic Jan 29, 2020

jgowdyelastic Jan 29, 2020

jgowdyelastic Jan 29, 2020 •

edited

Loading

alvarezmelissa87 Jan 29, 2020

jgowdyelastic Jan 29, 2020

alvarezmelissa87 Jan 29, 2020

jgowdyelastic Jan 29, 2020

alvarezmelissa87 Jan 29, 2020

jgowdyelastic Jan 29, 2020 •

edited

Loading

peteharverson left a comment

jgowdyelastic left a comment

alvarezmelissa87 commented Jan 29, 2020 •

edited

Loading

kibanamachine commented Jan 29, 2020

jgowdyelastic commented Jan 30, 2020

peteharverson left a comment

[ML] New Platform server shim: update calendar routes to use new platform router #56264

[ML] New Platform server shim: update calendar routes to use new platform router #56264

Conversation

alvarezmelissa87 commented Jan 29, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jan 29, 2020

jgowdyelastic Jan 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgowdyelastic Jan 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgowdyelastic Jan 29, 2020 • edited Loading

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

jgowdyelastic left a comment

Choose a reason for hiding this comment

alvarezmelissa87 commented Jan 29, 2020 • edited Loading

kibanamachine commented Jan 29, 2020

💚 Build Succeeded

History

jgowdyelastic commented Jan 30, 2020

peteharverson left a comment

Choose a reason for hiding this comment

alvarezmelissa87 commented Jan 29, 2020 •

edited

Loading

jgowdyelastic Jan 29, 2020 •

edited

Loading

jgowdyelastic Jan 29, 2020 •

edited

Loading

jgowdyelastic Jan 29, 2020 •

edited

Loading

alvarezmelissa87 commented Jan 29, 2020 •

edited

Loading