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.

Sign up for GitHub

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

Jump to bottom

[data.search.aggs] Remove service getters from agg types #61628

Merged
merged 24 commits into from
Apr 7, 2020
Merged

[data.search.aggs] Remove service getters from agg types #61628

merged 24 commits into from
Apr 7, 2020

Conversation

alexwizp
Copy link
Contributor

Part of #60333

Checklist

Delete any items that are not applicable to this PR.

For maintainers

alexwizp
alexwizp commented Mar 27, 2020
View reviewed changes
src/plugins/data/public/plugin.ts
@@ -100,6 +102,10 @@ export class DataPublicPlugin implements Plugin<DataPublicPluginSetup, DataPubli

expressions.registerFunction(esaggs);

const getInternalStartServices: getInternalStartServicesFn = () => ({
fieldFormats: getFieldFormats(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the future, we can remove getters/setters altogether, but now using this approach we can save support of two ways for passing dependencies and refactor all places without any rush.

@alexwizp alexwizp mentioned this pull request Mar 27, 2020
Closed
@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@elastic elastic deleted a comment from kibanamachine Mar 27, 2020
@elastic elastic deleted a comment from kibanamachine Mar 28, 2020
@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@lukeelmers
Copy link
Member

👍 This approach makes sense to me & feels like an improvement over what we have now

Even if we end up with a different solution in the long run, this has the advantage of consolidating all of the service getters so that we aren't importing from ../services all over the place, so it will make it easier to update later if necessary.

@alexwizp
Copy link
Contributor Author

@lizozom @ppisljar @streamich Could you please also review it and give your opinion. If you don't have any objections probably we can start to use this approach

@alexwizp alexwizp requested review from lizozom, ppisljar, juliocamarero and streamich and removed request for juliocamarero March 31, 2020 07:37
@alexwizp alexwizp added release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0 labels Apr 1, 2020
@alexwizp alexwizp self-assigned this Apr 1, 2020
@alexwizp alexwizp marked this pull request as ready for review April 1, 2020 07:41
@alexwizp alexwizp added the WIP Work in progress label Apr 2, 2020
@alexwizp alexwizp removed the WIP Work in progress label Apr 4, 2020
@alexwizp alexwizp changed the title [WIP][data.search.aggs] Remove service getters from agg types [data.search.aggs] Remove service getters from agg types Apr 4, 2020
@alexwizp alexwizp requested a review from lukeelmers April 4, 2020 09:57
@VladLasitsa
Copy link
Contributor

@elasticmachine merge upstream

lukeelmers
lukeelmers approved these changes Apr 6, 2020
View reviewed changes
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Code changes LGTM; did some smoke tests locally (Chrome macOS), and things seem to be working as expected.

I do think we should eventually consider passing specific services to each agg type rather than all of getInternalStartServices, however since more work will need to be done to prepare aggs to be available on the server anyway, I'm comfortable addressing this later.

Either way this PR is IMHO an improvement on what we had previously, because:

  1. It consolidates usage of service getters to one place, making them easy to swap out down the road.
  2. It updates the plumbing of each of the agg types so that dependencies are passed down rather than imported via a ./services module.

src/plugins/data/public/field_formats/mocks.ts Outdated Show resolved Hide resolved
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@alexwizp alexwizp merged commit 008b0fd into elastic:master Apr 7, 2020
kibana-app-arch automation moved this from Review in progress to Done in current release Apr 7, 2020
alexwizp added a commit to alexwizp/kibana that referenced this pull request Apr 7, 2020
@alexwizp @elasticmachine @VladLasitsa
[data.search.aggs] Remove service getters from agg types (elastic#61628)
2b1c217 
* [data.search.aggs] Remove service getters from agg types

Part of elastic#60333

* new portion of changes

* pass dependencies to MetricAgg Type through constructor

* update docs

* refactoring buckets

* remove unused mockDataServices

* Remove service getters from metrics

* Some fixes

* remove temporary code

* moved notifications to the getInternalStartServices

* fixed karma lock

* update docs

* Fixed tests

* fix broken CI

* fix PR comment

* fix typo

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Uladzislau Lasitsa <Uladzislau_Lasitsa@epam.com>
@alexwizp alexwizp mentioned this pull request Apr 7, 2020
Merged
alexwizp added a commit that referenced this pull request Apr 7, 2020
@alexwizp @elasticmachine @VladLasitsa
[data.search.aggs] Remove service getters from agg types (#61628) (#6…
a83455c 
…2762)

* [data.search.aggs] Remove service getters from agg types

Part of #60333

* new portion of changes

* pass dependencies to MetricAgg Type through constructor

* update docs

* refactoring buckets

* remove unused mockDataServices

* Remove service getters from metrics

* Some fixes

* remove temporary code

* moved notifications to the getInternalStartServices

* fixed karma lock

* update docs

* Fixed tests

* fix broken CI

* fix PR comment

* fix typo

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Uladzislau Lasitsa <Uladzislau_Lasitsa@epam.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Uladzislau Lasitsa <Uladzislau_Lasitsa@epam.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 7, 2020
@gmmorris
Merge branch 'master' into alerting/tls-disable
a328e16 
* master: (36 commits)
  [data.search.aggs] Remove service getters from agg types (elastic#61628)
  fixing APM internationalization (elastic#62757)
  fix: 🐛 correctly create error on no_matching_indices (elastic#61257)
  [Lens] Remove all legacy imports (elastic#62596)
  Add label for ace editor (elastic#62588)
  [ML] Show better file structure finder explanations (elastic#62316)
  Fix old pathes in eslintrc (elastic#62580)
  [Uptime] Improve Telemetry test (elastic#62428)
  [SIEM] Adds sort rules Cypress test (elastic#62700)
  [Uptime]Abstracted 'access:uptime-read' tag into a wrapper for… (elastic#62576)
  fixing bug (elastic#62577)
  [Maps] Allow updating requestType for ESGeoGridSource (elastic#62365)
  [Maps] do not show circle border when symbol size is zero (elastic#62644)
  [Maps] Always show current zoom level (elastic#62684)
  bc5 siem rules merge (elastic#62679)
  Revert "[Monitoring] Cluster state watch to Kibana alerting (elastic#61685)"
  Fix visual tests (elastic#62660)
  [Telemetry] update crypto packages (elastic#62469)
  [DOCS] Removed references to left (elastic#60807)
  [Maps] Move layers to np maps (elastic#61877)
  ...
@lukeelmers lukeelmers mentioned this pull request May 6, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukeelmers lukeelmers lukeelmers approved these changes

@VladLasitsa VladLasitsa Awaiting requested review from VladLasitsa

@lizozom lizozom Awaiting requested review from lizozom

@ppisljar ppisljar Awaiting requested review from ppisljar

@streamich streamich Awaiting requested review from streamich

Assignees

@alexwizp alexwizp

Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0
Projects
kibana-app-arch
  
Done in current release
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@alexwizp @lukeelmers @VladLasitsa @kibanamachine @elasticmachine