[ML] Server info service refactor by jgowdyelastic · Pull Request #50302 · elastic/kibana

jgowdyelastic · 2019-11-12T13:14:42Z

Removes new_job_defaults.ts in favour of /services/ml_server_info
renames the function loadNewJobDefaults to loadMlServerInfo as more than just new job defaults are being loaded.
new_job_defaults was being deep linked to from other files, so it's best that it's moved to a common location. it also makes sense that the cloud checks to added to this file.

This PR contains the refactoring part from #49861 which were originally dropped to avoid unnecessary churn in #50139

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 · 2019-11-12T13:14:45Z

Pinging @elastic/ml-ui (:ml)

darnautov · 2019-11-12T13:18:41Z

...lugins/ml/public/jobs/jobs_list/components/node_available_warning/node_available_warning.tsx

+            id="xpack.ml.jobsList.nodeAvailableWarning.noMLNodesAvailableDescription"
+            defaultMessage="There are no ML nodes available."
+          />
+          <br />


why not use div instead of br after inline elements?

darnautov · 2019-11-12T13:23:12Z

x-pack/legacy/plugins/ml/public/services/ml_server_info.test.ts

+
+describe('ml_server_info', () => {
+  beforeEach(async done => {
+    await loadMlServerInfo();


I guess it makes sense to move jest.mock for mlInfo here

darnautov · 2019-11-12T13:23:58Z

x-pack/legacy/plugins/ml/public/services/ml_server_info.test.ts

+    it('can get could deployment id', async done => {
+      expect(isCloud()).toBe(true);
+      expect(cloudDeploymentId()).toBe('85d666f3350c469e8c3242d76a7f459c');
+      done();


there is no async code inside

darnautov · 2019-11-12T13:25:06Z

...lugins/ml/public/jobs/jobs_list/components/node_available_warning/node_available_warning.tsx

-    );
  }
+
+  const id = cloudDeploymentId();


getCloudDeploymentId? 👀

i purposely left this change as every function in this service except loadMlServerInfo is a getter and changing one would mean i'd need to change all and that would be a lot of churn.
Does anyone else have an opinion on this? I don't mind changing all, it'd just touch a lot of files.

added getter renaming changes to this PR as all files which use the functions have already been touched for the include path change

elasticmachine · 2019-11-12T14:06:04Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: fe17494

walterra

LGTM, in the long run I think I'd prefer getCloudDeploymentId() but maybe it's better to do that rename in a dedicated PR.

jgowdyelastic · 2019-11-12T14:45:35Z

thinking about it, changing the names of these functions won't touch any additional files because i've already had to update all of the include paths to ml_server_info
so i'll add the renaming to this PR

peteharverson

LGTM

elasticmachine · 2019-11-12T15:27:42Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 4d404d7

elasticmachine · 2019-11-12T15:34:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: afcc4f8

alvarezmelissa87

LGTM ⚡️

jgowdyelastic · 2019-11-12T21:40:16Z

@elasticmachine merge upstream

elasticmachine · 2019-11-12T22:52:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: e28cd36

…a into server-info-refactor

elasticmachine · 2019-11-13T09:04:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: f198f6f

darnautov

LGTM 👍

* [ML] Server info service refactor * removing new job defaults * changes based on review * renaming all ml server info getter functions * missed a file

…ger-ace-theme * 'master' of github.com:elastic/kibana: (56 commits) [ML] Server info service refactor (elastic#50302) Remove internal platform types exports (elastic#50427) [APM] Document `apm_oss.metricsIndices` and `apm_oss.sourcemap… (elastic#50312) [Telemetry] Server side fetcher (elastic#50015) [SIEM] Detection engine placeholders (elastic#50220) [Uptime] Donut chart loader position centered vertically (elastic#50219) update telemetry banner notice text (elastic#50403) Fix aborting when searching without batching (elastic#49966) [Telemetry] Remove telemetry splash page and add conditional messaging (elastic#50189) Revert chromedriver update (elastic#50324) Remove deprecated argument include_type_name from ES calls (elastic#50285) [Maps] add settings to maps telemetry (elastic#50161) remove visualize loader (elastic#46910) Fix misuse of react-router and react-router-dom (elastic#50120) Move table-list-view to kibana-react (elastic#50046) [ML] Stats bar for data frame analytics (elastic#49464) [ML] Make navigation in tests more stable (elastic#50132) Migrate authorization subsystem to the new platform. (elastic#46145) Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063) Update dependency @elastic/charts to v14 (elastic#49947) ...

* [ML] Server info service refactor * removing new job defaults * changes based on review * renaming all ml server info getter functions * missed a file

jgowdyelastic added 2 commits November 12, 2019 12:59

[ML] Server info service refactor

4e976d0

removing new job defaults

fe17494

jgowdyelastic added review non-issue Indicates to automation that a pull request should not appear in the release notes :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Nov 12, 2019

jgowdyelastic requested review from alvarezmelissa87, darnautov, peteharverson and walterra November 12, 2019 13:14

jgowdyelastic requested a review from a team as a code owner November 12, 2019 13:14

jgowdyelastic self-assigned this Nov 12, 2019

darnautov reviewed Nov 12, 2019

View reviewed changes

changes based on review

afcc4f8

walterra approved these changes Nov 12, 2019

View reviewed changes

renaming all ml server info getter functions

4d404d7

peteharverson approved these changes Nov 12, 2019

View reviewed changes

alvarezmelissa87 approved these changes Nov 12, 2019

View reviewed changes

Merge branch 'master' into server-info-refactor

e28cd36

jgowdyelastic added 2 commits November 13, 2019 07:48

missed a file

c0eb620

Merge branch 'server-info-refactor' of github.com:jgowdyelastic/kiban…

f198f6f

…a into server-info-refactor

darnautov approved these changes Nov 13, 2019

View reviewed changes

jgowdyelastic merged commit 167dd7f into elastic:master Nov 13, 2019

jgowdyelastic deleted the server-info-refactor branch November 13, 2019 11:05

jgowdyelastic mentioned this pull request Nov 13, 2019

[7.x] [ML] Server info service refactor (#50302) #50466

Merged

jgowdyelastic added a commit that referenced this pull request Nov 13, 2019

[ML] Server info service refactor (#50302) (#50466)

5dadb0a

* [ML] Server info service refactor * removing new job defaults * changes based on review * renaming all ml server info getter functions * missed a file

jgowdyelastic mentioned this pull request Dec 4, 2019

[7.5] [ML] Server info service refactor (#50302) #52168

Merged

jgowdyelastic added backport pending v7.5.1 labels Dec 4, 2019

jgowdyelastic added a commit that referenced this pull request Dec 4, 2019

[ML] Server info service refactor (#50302) (#52168)

06de1ba

* [ML] Server info service refactor * removing new job defaults * changes based on review * renaming all ml server info getter functions * missed a file

jgowdyelastic removed the backport pending label Dec 4, 2019

Conversation

jgowdyelastic commented Nov 12, 2019

Checklist

For maintainers

Uh oh!

elasticmachine commented Nov 12, 2019

Uh oh!

darnautov Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

darnautov Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

darnautov Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

darnautov Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Nov 12, 2019

💔 Build Failed

Uh oh!

walterra left a comment

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic commented Nov 12, 2019

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Nov 12, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Nov 12, 2019

💔 Build Failed

Uh oh!

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic commented Nov 12, 2019

Uh oh!

elasticmachine commented Nov 12, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Nov 13, 2019

💚 Build Succeeded

Uh oh!

darnautov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jgowdyelastic Nov 12, 2019 •

edited

Loading