Move ui/agg_types in to shim data plugin#56353
Conversation
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
|
@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. |
|
Hey @lukeelmers |
|
@sulemanof I hadn't thought about the git history issue with the other files that still live inside 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 |
|
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 |
e614e74 to
15b3667
Compare
There was a problem hiding this comment.
We decided to create a namespace for static export ;)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Why we don't have a esLint error here, we do export from '../../../ui/public/agg_types' 3 times
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
alexwizp
left a comment
There was a problem hiding this comment.
LGTM! Good first step of refactoring App Config
sulemanof
left a comment
There was a problem hiding this comment.
LGTM, left a minor comment
There was a problem hiding this comment.
this and DateRangeKey are only used by field formater deserialization, which i am moving inside data plugin
There was a problem hiding this comment.
where is this used externally ?
There was a problem hiding this comment.
just in default editor i believe, and just used as a type.
There was a problem hiding this comment.
only type is used externally
There was a problem hiding this comment.
most (if not all) of theese are just types needed by default editor to build their agg type option to editor control map
There was a problem hiding this comment.
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.
* 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) ...
💔 Build FailedHistory
To update your PR or re-run it, just comment with: |
Summary
ui/agg_typesinto the legacy shim data pluginui/agg_typesfor BWCTODO in follow up PRs
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
strikethroughsto 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- [ ] Unit or functional tests were updated or added to match the most common scenarios- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately