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.

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] Typescripting client side endpoint functions #59928

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Mar 11, 2020

  • Converts all client side endpoint wrappers to typescript.
  • Refactors http service to introduce common code between the two http functions.
  • Fixes issue with query parameters being encoded when added to an endpoint path.

Most endpoint functions either return Promise<any> or Promise<specific type> where the type is defined in or imported into the file.
Future work is needed to complete these missing types so none of the functions return an any.
It would also be good to restructure the index.ts file to split it up into smaller files. It was the original endpoint file and so has grown to be very large and contains a variety of functions which could go into separate sub sections.

@jgowdyelastic jgowdyelastic self-assigned this Mar 12, 2020
@jgowdyelastic jgowdyelastic added :ml bug Fixes for quality problems that affect the customer experience chore non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes review labels Mar 12, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@jgowdyelastic jgowdyelastic marked this pull request as ready for review March 12, 2020 09:23
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner March 12, 2020 09:23
@jgowdyelastic jgowdyelastic force-pushed the typescripting-client-side-endpoint-functions branch from 8344c47 to 59e7c79 Compare March 12, 2020 09:35
Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

LGTM, just added suggestion about adding types for data frame analytics endpoints.

method: 'GET',
});
},
createDataFrameAnalytics(analyticsId: string, analyticsConfig: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use DataFrameAnalyticsConfig from data_frame_analytics/common/analytics.ts maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

the places where createDataFrameAnalytics is called aren't supplying a DataFrameAnalyticsConfig object.
It looks like a DeepPartial<DataFrameAnalyticsConfig>.
I've updated the expected type to be DeepPartial<DataFrameAnalyticsConfig>.

body,
});
},
explainDataFrameAnalytics(jobConfig: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use DataFrameAnalyticsConfig from data_frame_analytics/common/analytics.ts maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

this also needs to be a DeepPartial<DataFrameAnalyticsConfig>

body,
});
},
evaluateDataFrameAnalytics(evaluateConfig: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use DataFrameAnalyticsConfig from data_frame_analytics/common/analytics.ts maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

it looks the evaluate config is something different altogether. without wanting to create a new type here, i'm going to leave this as any

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

@jgowdyelastic jgowdyelastic force-pushed the typescripting-client-side-endpoint-functions branch from 060b9a0 to 3ecbba2 Compare March 12, 2020 10:13
Copy link
Contributor

@peteharverson peteharverson 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 LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

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 ⚡️

@jgowdyelastic jgowdyelastic merged commit bacc746 into elastic:master Mar 12, 2020
@jgowdyelastic jgowdyelastic deleted the typescripting-client-side-endpoint-functions branch March 12, 2020 13:31
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Mar 12, 2020
* [ML] Typescripting client side endpoint functions

* type clean up

* cleaning up http requests

* remove http generics

* better use of generics and type clean up

* removes some generics

* update comment

* updating data frame analytics types

* fixing type errors
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 12, 2020
* master: (45 commits)
  skip flaky suite (elastic#59717)
  UI Metrics use findAll to retrieve all Saved Objects (elastic#59891)
  [Discover] Migrate Context mocha tests to use Jest (elastic#59658)
  [Maps] Move redux reducers and store logic to NP (elastic#58294)
  rebalance x-pack groups (elastic#58930)
  [Discover] Reimplement $route.reload when index pattern changes (elastic#59877)
  [Upgrade Assistant Meta] Breaking changes issue template (elastic#59745)
  Skip CI based on changes in PR (elastic#59939)
  [ML] Transforms: Replace KqlFilterBar with QueryStringInput. (elastic#59723)
  [ML] Functional tests - stabilize date_nanos test (elastic#59986)
  [ML] Typescripting client side endpoint functions (elastic#59928)
  a11y tests on adding columns to discover table (elastic#59375)
  fix graph plugin config path (elastic#59540)
  fix vega config issues (elastic#59737)
  [Upgrade Assistant] Open And Close Slight Refactor (elastic#59890)
  [ML] Adding shared services to ml setup contract (elastic#59730)
  [Visualize] Fix linked search behavior (elastic#59690)
  [ML] Register NP ML plugin for Kibana management section. (elastic#59762)
  [Lens] Adds using queries/filters for field existence endpoint (elastic#59033)
  Delete FilterStateManager and QueryFilter :-D (elastic#59872)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 13, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

jgowdyelastic added a commit that referenced this pull request Mar 13, 2020
* [ML] Typescripting client side endpoint functions

* type clean up

* cleaning up http requests

* remove http generics

* better use of generics and type clean up

* removes some generics

* update comment

* updating data frame analytics types

* fixing type errors

Co-authored-by: Elastic Machine <[email protected]>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience chore :ml non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes review v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants