[ML] Server info service refactor #50302
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jgowdyelastic
added
review
non-issue
jgowdyelastic
requested review from
walterra,
darnautov,
alvarezmelissa87 and
peteharverson
|
Pinging @elastic/ml-ui (:ml) |
darnautov
reviewed
Nov 12, 2019
| id="xpack.ml.jobsList.nodeAvailableWarning.noMLNodesAvailableDescription" | ||
| defaultMessage="There are no ML nodes available." | ||
| /> | ||
| <br /> |
There was a problem hiding this comment.
why not use div instead of br after inline elements?
|
|
||
| describe('ml_server_info', () => { | ||
| beforeEach(async done => { | ||
| await loadMlServerInfo(); |
There was a problem hiding this comment.
I guess it makes sense to move jest.mock for mlInfo here
Comment on lines
36
to
39
| it('can get could deployment id', async done => { | ||
| expect(isCloud()).toBe(true); | ||
| expect(cloudDeploymentId()).toBe('85d666f3350c469e8c3242d76a7f459c'); | ||
| done(); |
There was a problem hiding this comment.
there is no async code inside
| } | ||
|
|
||
| const id = cloudDeploymentId(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
added getter renaming changes to this PR as all files which use the functions have already been touched for the include path change
💔 Build Failed |
walterra
approved these changes
Nov 12, 2019
|
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 |
💔 Build Failed |
💔 Build Failed |
|
@elasticmachine merge upstream |
💔 Build Failed |
…a into server-info-refactor
💚 Build Succeeded |
jgowdyelastic
added a commit
to jgowdyelastic/kibana
that referenced
this pull request
Nov 13, 2019
* [ML] Server info service refactor * removing new job defaults * changes based on review * renaming all ml server info getter functions * missed a file
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Nov 13, 2019
…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) ...
jgowdyelastic
added a commit
that referenced
this pull request
Nov 13, 2019
jgowdyelastic
added a commit
to jgowdyelastic/kibana
that referenced
this pull request
Dec 4, 2019
* [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 a commit
that referenced
this pull request
Dec 4, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes
new_job_defaults.tsin favour of/services/ml_server_inforenames the function
loadNewJobDefaultstoloadMlServerInfoas more than just new job defaults are being loaded.new_job_defaultswas 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
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] 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~~- [ ] This was checked for keyboard-only and screenreader accessibility
For maintainers
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately