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

Move timefilter and timehistory instantiation ⇒ data plugin #44783

Merged
merged 24 commits into from
Sep 24, 2019

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Sep 4, 2019

Summary

Part of #44377

  • Added timefilter as a dependency to FilterService
  • Added timeHistory as a dependency to QueryBar
  • Pass timefilter as a dependency to changeTimeFilter and onBrushEvent functions
  • Added TimefilterContract and TimeHistoryContract types
  • Replaced isAutoRefreshSelectorEnabled and isTimeRangeSelectorEnabled public members of timefilter with appropriate get functions.
  • Made timefilter.getForceNow private
  • Created timefilter mock and updated all relevant tests.

Dev Docs

Move value_suggestions into NP and expose data.getSuggestions on start contract.

In old platform:

    import { npStart } from 'ui/new_platform';
    const timefilter: TimefilterContract = npStart.plugins.data.timefilter.timefilter;
    const timeHistory: TimeHistoryContract = npStart.plugins.data.timefilter.timeHistory;

In new platform:

    public start(core, plugins) {
       ...
       const timefilter: TimefilterContract = plugins.data.timefilter.timefilter;
       const timeHistory: TimeHistoryContract = plugins.data.timefilter.timeHistory;
    }

Also, in timefilter the following API changes were made:

  • Replaced timefilter.isAutoRefreshSelectorEnabled member with timefilter.isAutoRefreshSelectorEnabled()
  • Replaced timefilter.isTimeRangeSelectorEnabled with timefilter.isTimeRangeSelectorEnabled()
  • timefilter.getForceNow is now private

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@lizozom lizozom added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Sep 4, 2019
@lizozom lizozom self-assigned this Sep 4, 2019
@lizozom lizozom changed the title Move timefilter and timehistory instantiation into data plugin Move timefilter and timehistory instantiation ⇒ data plugin Sep 4, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lizozom lizozom force-pushed the newplatform/timefilter/move-data-2 branch from 31da752 to f7adb55 Compare September 4, 2019 18:18
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lizozom
Copy link
Contributor Author

lizozom commented Sep 23, 2019

Copying @peteharverson 's feedback here:

I see you are switching back the mocks in that PR to use ui/timefilter. I am a bit confused as Josh changed them to use ui/new_platform quite recently

@lizozom
Copy link
Contributor Author

lizozom commented Sep 23, 2019

@peteharverson

ML uses timefilter extensively, and moving it into data plugin, introduces additional dependencies that needed to be mocked, just by importing timefilter.

(For example ui/registry/field_formats,ui/persisted_logs, ui/value_suggestions and more.)
(On a side note, these dependencies are going away one by one!)

I could mock them independently, but since most tests don't even need timefilter, I chose to mock the whole ui/timefilter import. A side effect of this, was making the ui/new_platform mock redundant, so I removed it.

i.e. mocking ui/timefilter mocks the data plugin, which uses mocked ui/new_platform core plugins anyway.

I hope this makes it clearer.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lizozom lizozom added the review label Sep 23, 2019
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM with one minor comment, tested on chrome linux.

the issue with not reloading settings can be addressed in separate PR as its also present on master


export class TimefilterService {
public setup({ uiSettings }: TimeFilterServiceDependencies): TimefilterSetup {
const timefilterConfig = {
Copy link
Member

Choose a reason for hiding this comment

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

hmm not sure if this is (was) working fine:

  • you start kibana, TimefilterService gets started and reads the uiSettings
  • you go to management -> kibana advanced settings and you change them
  • they don't apply until you refresh your browser (to reload kibana)

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 ML, and edits LGTM.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lizozom lizozom merged commit c28ce03 into elastic:master Sep 24, 2019
lizozom pushed a commit to lizozom/kibana that referenced this pull request Sep 24, 2019
…44783)

* Create timefilter service

* Pass timehistory as a dependency to query bar

* Updated timefilter tests and mocks

* Fix jest test

* Fixed bad import path

* updated xpack test

* make history optional on query bar

* Fixed chart_utils test

* Fixed page.test by mocking timefilter

* Fixed explorer_charts_container_service test by mocking timefilter

* Fixed explorer_chart_distribution test by mocking timefilter

* Fixed explorer_chart_single_metric test by mocking timefilter

* Fixed explorer_charts_container test by mocking timefilter and removing some unneeded initialization

* Fixed explorer_swimlane tesy by mocking timefilter

* Added missing functions to uiTimefilterMock to fix top_nav test

* Fixed timeseries_chart test by mocking timefilter

* remove bad merge

* mock field formats

* mock field formats

* rename timefilter boolean getters

* fix ml mocks

* ml mocks
lizozom pushed a commit that referenced this pull request Sep 24, 2019
…46467)

* Create timefilter service

* Pass timehistory as a dependency to query bar

* Updated timefilter tests and mocks

* Fix jest test

* Fixed bad import path

* updated xpack test

* make history optional on query bar

* Fixed chart_utils test

* Fixed page.test by mocking timefilter

* Fixed explorer_charts_container_service test by mocking timefilter

* Fixed explorer_chart_distribution test by mocking timefilter

* Fixed explorer_chart_single_metric test by mocking timefilter

* Fixed explorer_charts_container test by mocking timefilter and removing some unneeded initialization

* Fixed explorer_swimlane tesy by mocking timefilter

* Added missing functions to uiTimefilterMock to fix top_nav test

* Fixed timeseries_chart test by mocking timefilter

* remove bad merge

* mock field formats

* mock field formats

* rename timefilter boolean getters

* fix ml mocks

* ml mocks
@rayafratkina
Copy link
Contributor

@lizozom can you please add a DevDocs section?

@lizozom lizozom deleted the newplatform/timefilter/move-data-2 branch November 14, 2019 13:03
@elasticmachine
Copy link
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. review v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants