Skip to content

Comments

[Console] Refactor and cleanup of public and server#60513

Merged
jloleysens merged 6 commits intoelastic:masterfrom
jloleysens:refactor/console/cleanup
Mar 19, 2020
Merged

[Console] Refactor and cleanup of public and server#60513
jloleysens merged 6 commits intoelastic:masterfrom
jloleysens:refactor/console/cleanup

Conversation

@jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Mar 18, 2020 •
edited
Loading

Summary

This cleanup targets:

  • Remove of Ace in public lib folder (specifically autocomplete.ts)
  • Refactor loading of console spec files (in a new service)

How to test

  • Try booting Console OSS and Xpack and check that the expected spec is there (like _ml endpoints in Xpack).
  • Check for any console errors at startup
  • Most of the changes are targetting spec and autocomplete, so anything that the user might expect those to do, that you can think of would be great to test.

@jloleysens
Clean up use of ace in autocomplete in public
fe329a5 
Remove ace from lib/autocomplete.ts and set up hooking up of ace
in legacy_core_editor. Also remove use of ace mocks in tests.
@jloleysens
Added TODO in lib/kb (console public)
834c0aa
@jloleysens
Server-side cleanup
b16eb50 
Refactored the loading of spec into a new SpecDefinitionsService.
In this way, state can be contained inside of the service as much
as possible. Also converted all JS spec to TS and updated the
Console plugin contract so that processors (which alter loaded
spec) happen at plugin "start" phase.
@jloleysens jloleysens added Feature:Console Dev Tools Console Feature Feature:Dev Tools v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 18, 2020
@jloleysens jloleysens requested a review from sebelga March 18, 2020 13:51
@elasticmachine
Copy link
Contributor

elasticmachine commented Mar 18, 2020

Pinging @elastic/es-ui (Team:Elasticsearch UI)

jloleysens
jloleysens commented Mar 18, 2020
View reviewed changes
src/plugins/console/public/application/models/sense_editor/__tests__/integration.test.js
done();
return;
}
senseEditor.autocomplete._test.getCompletions(senseEditor, null, cursor, '', function(
Copy link
Contributor Author

@jloleysens jloleysens Mar 18, 2020

Choose a reason for hiding this comment

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

Preferably ignore these changes for now, they are mostly whitespace and the spec is hard to read. We should definitely address this at some point but for now they provide really decent coverage.

Copy link
Contributor

@sebelga sebelga Mar 19, 2020

Choose a reason for hiding this comment

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

No worries, Github "Hide whitespace changes" makes this easy to parse 😊

@cjcenizal cjcenizal mentioned this pull request Mar 19, 2020
Merged
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into refactor/cons…
3c1d020 
…ole/cleanup

* 'master' of github.com:elastic/kibana: (47 commits)
  [Remote clusters] Update copy (elastic#60382)
  Sort by name when fetching alerts and connectors (elastic#60506)
  Make slack param validation handle empty messages (elastic#60468)
  [Alerting] Adds navigation by consumer and alert type to alerting (elastic#58997)
  Introduce search interceptor (elastic#60523)
  [ML] Add functional tests for file data visualizer (elastic#60413)
  [APM] Optimize service map query (elastic#60412)
  [SIEM][Detection Engine] Adds lists feature flag and list values to the REST interfaces
  Enhancement/update esdocs datasource (elastic#59512)
  [junit] only include stdout in report for failures (elastic#60530)
  Update dependency nock to v12 (elastic#60422)
  upgrade execa to get stdout/stderr in error messages (elastic#60537)
  skip flaky suite (elastic#60471)
  [Ingest] Agent Config Details - Data sources list ui (elastic#60429)
  [SIEM] Create ML Rules (elastic#58053)
  skip flaky suite (elastic#60559)
  fix agent type (elastic#60554)
  Fixed default message for index threshold includes both threshold values (elastic#60545)
  [Ingest] Add support for `yaml` field types (elastic#60440)
  Solved the issue for a GROUP BY expression validation (elastic#60558)
  ...
sebelga
sebelga approved these changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally both oss and x-pack and could not spot any regression. 👍

src/plugins/console/public/application/models/sense_editor/__tests__/integration.test.js
done();
return;
}
senseEditor.autocomplete._test.getCompletions(senseEditor, null, cursor, '', function(
Copy link
Contributor

@sebelga sebelga Mar 19, 2020

Choose a reason for hiding this comment

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

No worries, Github "Hide whitespace changes" makes this easy to parse 😊

src/plugins/console/public/lib/autocomplete/autocomplete.ts

return {
getCompletions,
// TODO: This needs to be cleaned up
Copy link
Contributor

@sebelga sebelga Mar 19, 2020

Choose a reason for hiding this comment

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

Yes! Exporting a special object for testing indicates some spaghetti code somewhere 😊

@jloleysens
Small refactor
5886a2a 
- Updated naming of argument variable in registerAutocompleter
- Refactored the SpecDefinitionsService to handle binding of
it's own functions
@kibanamachine
Copy link
Contributor

kibanamachine commented Mar 19, 2020

💚 Build Succeeded

History

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

@jloleysens jloleysens merged commit 304b322 into elastic:master Mar 19, 2020
@jloleysens jloleysens deleted the refactor/console/cleanup branch March 19, 2020 16:32
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 19, 2020
@jloleysens
[Console] Refactor and cleanup of public and server (elastic#60513)
0d39f8b 
* Clean up use of ace in autocomplete in public

Remove ace from lib/autocomplete.ts and set up hooking up of ace
in legacy_core_editor. Also remove use of ace mocks in tests.

* Added TODO in lib/kb (console public)

* Server-side cleanup

Refactored the loading of spec into a new SpecDefinitionsService.
In this way, state can be contained inside of the service as much
as possible. Also converted all JS spec to TS and updated the
Console plugin contract so that processors (which alter loaded
spec) happen at plugin "start" phase.

* Fix types

* Small refactor

- Updated naming of argument variable in registerAutocompleter
- Refactored the SpecDefinitionsService to handle binding of
it's own functions
@jloleysens jloleysens mentioned this pull request Mar 19, 2020
Merged
jloleysens added a commit that referenced this pull request Mar 20, 2020
@jloleysens @elasticmachine
[Console] Refactor and cleanup of public and server (#60513) (#60652)
491933d 
* Clean up use of ace in autocomplete in public

Remove ace from lib/autocomplete.ts and set up hooking up of ace
in legacy_core_editor. Also remove use of ace mocks in tests.

* Added TODO in lib/kb (console public)

* Server-side cleanup

Refactored the loading of spec into a new SpecDefinitionsService.
In this way, state can be contained inside of the service as much
as possible. Also converted all JS spec to TS and updated the
Console plugin contract so that processors (which alter loaded
spec) happen at plugin "start" phase.

* Fix types

* Small refactor

- Updated naming of argument variable in registerAutocompleter
- Refactored the SpecDefinitionsService to handle binding of
it's own functions

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@sebelga sebelga sebelga approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:Console Dev Tools Console Feature Feature:Dev Tools release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v7.7.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jloleysens @elasticmachine @kibanamachine @sebelga