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

Cleanup OSS code from visualizations wizard #89092

Merged
merged 10 commits into from
Jan 27, 2021

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Jan 25, 2021

Summary

This PR cleans up the code that was related to the OSS functionality of the visualizations wizard component that was added here. It has to do with the two disabled cards that appear on the Oss version for Lens and Maps.
wizard_oss

Specifically:

  • Removes the lens_oss and maps_oss plugins that registered the oss aliases.
  • Maps and Lens plugins no need to unregister the oss alias (as it is not registered anymore)
  • Removes the promoTooltip and disabled props from the VisTypeAlias as they were used only for the OSS
  • Removes the copyFromRoot from kbn/config as we are not going to need it anymore
  • Removes the functional test for OSS

Note:
I will not remove the unRegisterAlias method as it may be useful for some cases (see #88723)

Checklist

Delete any items that are not applicable to this PR.

@stratoula stratoula added release_note:skip Skip the PR/issue when compiling release notes Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.12.0 v8.0.0 labels Jan 25, 2021
@stratoula stratoula added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Jan 25, 2021
@stratoula stratoula marked this pull request as ready for review January 25, 2021 17:20
@stratoula stratoula requested a review from a team January 25, 2021 17:20
@stratoula stratoula requested review from a team as code owners January 25, 2021 17:20
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

changes to bundle limits LGTM

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.

Code LGTM, tested locally, works well 🎉
Just have a small concern around functional tests

test/functional/apps/visualize/index.ts Show resolved Hide resolved
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Kibana app part LGTM, tested in chrome and works as expected

'Area',
'Coordinate Map',
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we won't test whether coordinate map and region map show up at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On this test yes but we already test it here
Not directly, but this test opens the wizard and clicks on the region map. Unfortunately, when the ci stops running the tests for OSS the above test won't also run.
if (isOss) { loadTestFile(require.resolve('./_tile_map')); loadTestFile(require.resolve('./_region_map')); }
We have to discuss this with the maps team.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, makes sense

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM for kbn-config changes. Thanks for reverting copyFromRoot in this PR.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lensOss 7 - -7
mapsOss 7 - -7
total -14

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 851.9KB 851.8KB -1.0B
maps 2.6MB 2.6MB -3.0B
visualizations 58.0KB 56.9KB -1.1KB
total -1.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
lens 34.4KB 34.0KB -361.0B
lensOss 4.5KB - -4.5KB
maps 138.6KB 138.2KB -351.0B
mapsOss 4.2KB - -4.2KB
total -9.4KB

History

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

@stratoula stratoula merged commit 64e9cf0 into elastic:master Jan 27, 2021
stratoula added a commit to stratoula/kibana that referenced this pull request Jan 27, 2021
* Cleanup OSS code from visualizations wizard

* Remove unecessary translations

* Remove unused translation

* Fix functional test

* Disable the functional test for OSS

* Remove from oss correctly :D

* Fix ci
# Conflicts:
#	.github/CODEOWNERS
#	docs/developer/plugin-list.asciidoc
#	packages/kbn-optimizer/limits.yml
stratoula added a commit that referenced this pull request Jan 27, 2021
* Cleanup OSS code from visualizations wizard (#89092)

* Cleanup OSS code from visualizations wizard

* Remove unecessary translations

* Remove unused translation

* Fix functional test

* Disable the functional test for OSS

* Remove from oss correctly :D

* Fix ci
# Conflicts:
#	.github/CODEOWNERS
#	docs/developer/plugin-list.asciidoc
#	packages/kbn-optimizer/limits.yml

* fix plugin list docs

* Change plugins docs list
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 27, 2021
…y-tests

* 'master' of github.com:elastic/kibana: (276 commits)
  [Telemetry] Settings Collector: redact sensitive reported values (elastic#88675)
  [CI] Combines Jest test jobs (elastic#85850)
  [Upgrade Assistant] Migrate server to new es-js client (elastic#89207)
  Migrate maps_legacy, maps_oss, region_map, and tile_map plugions to TS projects (elastic#89351)
  [Vega Docs] Add experimental flag on the vega maps title (elastic#89402)
  Increase the time needed to locate the save viz toast (elastic#89301)
  [Enterprise Search] Add links to doc links service (elastic#89260)
  Fixed regex bug in Safari (elastic#89399)
  [Lens] Fix indexpattern checks for missing references (elastic#88840)
  [Lens] Clean up usage collector (elastic#89109)
  update apm index pattern (elastic#89395)
  [APM] Upgrade ES client (elastic#86594)
  Enable v2 so migrations, disable in FTR tests (elastic#89297)
  [Search Sessions] Make search session indicator UI opt-in, refactor per-app capabilities (elastic#88699)
  Cleanup OSS code from visualizations wizard (elastic#89092)
  [APM] Optimize API test order (elastic#88654)
  Rename conversion function, extract to module scope and add tests. (elastic#89018)
  [core.logging] Add ops logs to the KP logging system (elastic#88070)
  chore(NA): improve ts build refs performance on kbn bootstrap (elastic#89333)
  skip flaky suite (elastic#89379)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/timeline/timeline.tsx
#	x-pack/test/accessibility/config.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 27, 2021
…ana into task-manager/shift-on-trend

* 'task-manager/shift-on-trend' of github.com:gmmorris/kibana: (74 commits)
  [Metrics UI] Fix Host Overview boxes in Host Detail page (elastic#89299)
  [Telemetry] Settings Collector: redact sensitive reported values (elastic#88675)
  [CI] Combines Jest test jobs (elastic#85850)
  [Upgrade Assistant] Migrate server to new es-js client (elastic#89207)
  Migrate maps_legacy, maps_oss, region_map, and tile_map plugions to TS projects (elastic#89351)
  [Vega Docs] Add experimental flag on the vega maps title (elastic#89402)
  Increase the time needed to locate the save viz toast (elastic#89301)
  [Enterprise Search] Add links to doc links service (elastic#89260)
  Fixed regex bug in Safari (elastic#89399)
  [Lens] Fix indexpattern checks for missing references (elastic#88840)
  [Lens] Clean up usage collector (elastic#89109)
  update apm index pattern (elastic#89395)
  [APM] Upgrade ES client (elastic#86594)
  Enable v2 so migrations, disable in FTR tests (elastic#89297)
  [Search Sessions] Make search session indicator UI opt-in, refactor per-app capabilities (elastic#88699)
  Cleanup OSS code from visualizations wizard (elastic#89092)
  [APM] Optimize API test order (elastic#88654)
  Rename conversion function, extract to module scope and add tests. (elastic#89018)
  [core.logging] Add ops logs to the KP logging system (elastic#88070)
  chore(NA): improve ts build refs performance on kbn bootstrap (elastic#89333)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants