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

Move ui/agg_types in to shim data plugin #56353

Merged
merged 10 commits into from
Feb 5, 2020
Merged

Move ui/agg_types in to shim data plugin #56353

merged 10 commits into from
Feb 5, 2020

Conversation

lukeelmers
Copy link
Member

@lukeelmers lukeelmers commented Jan 30, 2020 •
edited
Loading

Summary

  • Moves the contents of ui/agg_types into the legacy shim data plugin
  • Re-exports contracts ui/agg_types for BWC
  • Creates dedicated interfaces for classes commonly being used as types: IAggConfig, IAggConfigs, IAggType, IFieldParamType, IMetricAggType
    • Right now these are just re-exporting the class as a type; eventually we should put more detailed typings in place.

TODO in follow up PRs

  • Review & clean up the exposed contracts. Currently this just putting anything that's being used elsewhere in Kibana in the contract, and anything that contains state is being stuck in as part of the lifecycle contract. We should be able to make many of these items static & export them statically.
  • Removal of legacy services in preparation for moving to new platform.

Note for reviewers: It will be much easier to review this commit-by-commit instead of attempting to look at the diff.

Note for QA: This should be a fully backwards-compatible change and shouldn't affect any functionality across Kibana.

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

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately

@lukeelmers lukeelmers added WIP Work in progress Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v8.0.0 Team:AppArch Feature:NP Migration v7.7.0 labels Jan 30, 2020
@lukeelmers lukeelmers self-assigned this Jan 30, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lukeelmers lukeelmers added review and removed WIP Work in progress labels Jan 31, 2020
@lukeelmers lukeelmers marked this pull request as ready for review January 31, 2020 00:41
@lukeelmers lukeelmers requested a review from a team January 31, 2020 00:41
@lukeelmers lukeelmers requested a review from a team as a code owner January 31, 2020 00:41
@lukeelmers
Copy link
Member Author

lukeelmers commented Jan 31, 2020 •
edited
Loading

@ppisljar @alexwizp @sulemanof @flash1293 Would appreciate a review from whomever has the chance, especially as this PR will be prone to merge conflicts over time since it touches a lot of files. (Including conflicts with #55351, which should merge first).

You'll definitely want to review this commit-by-commit rather than trying to read the diff.

@sulemanof
Copy link
Contributor

Hey @lukeelmers
My worry is about saving a bunch of files with re-exports in there in ui/agg_types.
In that case we'll lose git history of it and it could lead to regressions while refactoring those (it will be more difficult to find a particular commit to get an intension of a feature).
Wouldn't be better to change imports of those from the new place? I think the most places are in vis_default_editor plugin, so it shouldn't be a big problem.

@lukeelmers
Copy link
Member Author

@sulemanof I hadn't thought about the git history issue with the other files that still live inside ui/agg_types, but I can certainly go through and remove those so that we're left with a single index.ts, and update imports accordingly.

Primarily I did it this way to avoid needing to update downstream imports in this PR (which is already big). However, with the interface changes I needed to make I already had to touch a bunch of import statements anyway, so might as well just export everything from ui/agg_types/index.ts for now.

@lukeelmers
Copy link
Member Author

Updated on latest master, and removed extra nested files as suggested by @sulemanof

Still need to wait for #55351 to merge so those changes can be incorporated here, but feel free to review in the meantime

@lukeelmers lukeelmers force-pushed the aggs/move branch 2 times, most recently from e614e74 to 15b3667 Compare January 31, 2020 23:21
alexwizp
alexwizp reviewed Feb 3, 2020
View reviewed changes
src/legacy/core_plugins/data/public/search/aggs/index.ts
} from './metrics/lib/sibling_pipeline_agg_helper';

// static code
export { AggParamType } from './param_types/agg';
Copy link
Contributor

Choose a reason for hiding this comment

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

We decided to create a namespace for static export ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

My plan was to revisit the actual structure of the contracts for this service in a follow-up PR, where I'd update the structure of both static & runtime contracts at once.

Also Liza discovered a potential issue with namespaces & generated documentation. It is being discussed here whether they will still be a viable option for us, so if we have an answer to that soon I can update the implementation accordingly.

alexwizp
alexwizp reviewed Feb 3, 2020
View reviewed changes
src/legacy/core_plugins/visualizations/public/legacy_imports.ts Outdated
} from '../../../ui/public/agg_types/buckets/date_histogram';
export { IAggConfig } from '../../../ui/public/agg_types';
export { IAggConfigs } from '../../../ui/public/agg_types';
export { isDateHistogramBucketAggConfig, setBounds } from '../../../ui/public/agg_types';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we don't have a esLint error here, we do export from '../../../ui/public/agg_types' 3 times

image

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, thanks for pointing this out. they were a byproduct of the way i was grepping to update import paths across kibana -- some of them used to be importing from different paths but have all been changed, and i guess the linter wasn't complaining about it. i went ahead and updated this file.

Copy link
Contributor

@sulemanof sulemanof Feb 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if this is really worthy to highlight, but there are still some places with the same imports:
src/legacy/ui/public/visualize/loader/pipeline_helpers/utilities.ts
x-pack/legacy/plugins/rollup/public/legacy.ts
x-pack/legacy/plugins/rollup/public/legacy_imports.ts
src/legacy/core_plugins/vis_default_editor/public/legacy_imports.ts

@lukeelmers lukeelmers mentioned this pull request Feb 3, 2020
Merged
@lukeelmers lukeelmers removed the review label Feb 3, 2020
sulemanof
sulemanof approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@sulemanof sulemanof left a comment

Choose a reason for hiding this comment

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

LGTM, left a minor comment

This was referenced Feb 4, 2020
Closed
Merged
ppisljar
ppisljar approved these changes Feb 4, 2020
View reviewed changes
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.

code LGTM

src/legacy/core_plugins/data/public/index.ts
IAggType,
IFieldParamType,
IMetricAggType,
IpRangeKey,
Copy link
Member

Choose a reason for hiding this comment

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

this and DateRangeKey are only used by field formater deserialization, which i am moving inside data plugin

src/legacy/core_plugins/data/public/index.ts
export {
SavedQueryAttributes,
SavedQuery,
SavedQueryTimeFilter,
} from '../../../../plugins/data/public';
export {
// agg_types
AggParam,
Copy link
Member

Choose a reason for hiding this comment

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

where is this used externally ?

Copy link
Member Author

Choose a reason for hiding this comment

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

just in default editor i believe, and just used as a type.

src/legacy/core_plugins/data/public/index.ts
export {
// agg_types
AggParam,
AggParamOption,
Copy link
Member

Choose a reason for hiding this comment

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

only type is used externally

src/legacy/core_plugins/data/public/index.ts
IMetricAggType,
IpRangeKey,
ISchemas,
OptionedParamEditorProps,
Copy link
Member

Choose a reason for hiding this comment

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

only type is needed

Copy link
Member

Choose a reason for hiding this comment

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

same for the one below

src/legacy/core_plugins/data/public/index.ts
propFilter,
Schema,
Schemas,
siblingPipelineType,
Copy link
Member

Choose a reason for hiding this comment

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

most (if not all) of theese are just types needed by default editor to build their agg type option to editor control map

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, in the next pass I want to go through and strip away more of this stuff. For this first PR I kept things focused on just re-exporting absolutely anything that was needed outside of the data plugin, but there will be plenty more cleanup to follow.

@lukeelmers lukeelmers merged commit 5f93864 into elastic:master Feb 5, 2020
@lukeelmers lukeelmers deleted the aggs/move branch February 5, 2020 00:35
@lukeelmers lukeelmers mentioned this pull request Feb 5, 2020
Merged
lukeelmers added a commit that referenced this pull request Feb 5, 2020
@lukeelmers
Move ui/agg_types in to shim data plugin (#56353) (#56815)
1a47927
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 5, 2020
@gmmorris
Merge branch 'master' into alerting/get-alert-state
a34c0cb 
* master: (23 commits)
  Properly handle password change for users authenticated with provider other than `basic`. (elastic#55206)
  Improve pull request template proposal (elastic#56756)
  Only change handlers as the element changes (elastic#56782)
  [SIEM][Detection Engine] Final final rule changes (elastic#56806)
  [SIEM][Detection Engine] critical blocker, wrong ilm policy, need to match beats ilm policy
  Move ui/agg_types in to shim data plugin (elastic#56353)
  [SIEM] Fixes Signals count spinner (elastic#56797)
  [docs] Update upgrade version path (elastic#56658)
  [Canvas] Use unique Id for Canvas Embeddables (elastic#56783)
  [Rollups] Adjust max width for job detail panel (elastic#56674)
  Prevent http client from converting our form data (elastic#56772)
  Disable creating alerts client instances when ESO plugin is using an ephemeral encryption key (elastic#56676)
  Bumps terser-webpack-plugin to 2.3.4 (elastic#56662)
  Advanced settings component registry ⇒ kibana platform plugin (elastic#55940)
  [Endpoint] EMT-67: add kql support for endpoint list (elastic#56328)
  Implement UI for Create Alert form  (elastic#55232)
  Fix: Filter pill base coloring (elastic#56761)
  fix open close signal on detail page (elastic#56757)
  [Search service] Move loadingCount to sync search strategy (elastic#56335)
  Rollup TSVB integration: Add test and fix warning text (elastic#56639)
  ...
@lukeelmers lukeelmers mentioned this pull request Feb 10, 2020
Closed
@kibanamachine
Copy link
Contributor

💔 Build Failed

History

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

@lukeelmers lukeelmers added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. labels Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sulemanof sulemanof sulemanof approved these changes

@alexwizp alexwizp alexwizp approved these changes

@ppisljar ppisljar ppisljar approved these changes

@flash1293 flash1293 Awaiting requested review from flash1293

Assignees

@lukeelmers lukeelmers

Labels
Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lukeelmers @elasticmachine @sulemanof @kibanamachine @ppisljar @alexwizp