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

ES UI new platform cleanup #64332

Merged
merged 10 commits into from
Apr 28, 2020
Merged

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Apr 23, 2020

This PR removes the stub remote_clusters, index_management, and upgrade_assistant legacy plugins. Changes to note:

@cjcenizal cjcenizal added Feature:Index Management Index and index templates UI Feature:CCR and Remote Clusters v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.8.0 labels Apr 23, 2020
@cjcenizal cjcenizal requested a review from sebelga April 23, 2020 17:03
@cjcenizal cjcenizal requested review from a team as code owners April 23, 2020 17:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@cjcenizal cjcenizal requested a review from cchaos April 23, 2020 17:03
@cjcenizal
Copy link
Contributor Author

@cchaos I understand the reasoning behind extracting styles out of the root index.scss file and having them live next to the components that own them but in cases like these, where we have only a couple tightly-focused hacks, I think it's better to keep them in a central location where it's easier to discover them (and remove them once the're no longer necessary).

@cchaos
Copy link
Contributor

cchaos commented Apr 23, 2020

When they are hacks, we actually use a file named _hacks.scss. Example: https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/kibana/public/management/_hacks.scss

@cjcenizal cjcenizal requested a review from a team as a code owner April 23, 2020 18:39
@cjcenizal
Copy link
Contributor Author

@jloleysens Could you take a look at the Upgrade Assistant changes (91957739bf40aa9803dd11a24983ecf91ad770d3)? I'm not sure how to test these.

@pgayvallet Could you take a look at my change to SavedObjectsCoreFieldMapping?

…es to NP plugin.

- Update SavedObjectsCoreFieldMapping with null_field parameter.
@cjcenizal
Copy link
Contributor Author

Thanks for tip, @cchaos! Can you take another look?

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Sure, that's fine

Copy link
Contributor

@joshdover joshdover 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! FYI you'll need to regenerate and commit the core API docs:

node scripts/check_published_api_changes.js --docs --accept --filter=core/server

@cjcenizal
Copy link
Contributor Author

Thanks @joshdover, done.

name: REINDEX_OP_TYPE,
hidden: false,
namespaceType: 'agnostic',
mappings: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new Saved Object types API disallows dynamic mapping, which the original type definitions depended upon, so I had to add a bunch of new mappings to get the tests passing.

Copy link
Contributor

@pgayvallet pgayvallet Apr 27, 2020

Choose a reason for hiding this comment

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

If dynamic mappings in SO type are really a need, we can discuss about enabling it (actually it's only 'disabled' in the TS type, forcing the dynamic: true value does work, see (but please don't do):

// we don't want to allow `true` in the public `SavedObjectsTypeMappingDefinition` type, however
// this is needed for the config that is kinda a special type. To avoid adding additional internal types
// just for this, we hardcast to any here.
dynamic: true as any,

It's just that when we discussed about it, we did not find any valid argument to allow this value on the public API as a SO type mapping is meant to be exhaustive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Pierre! I don't think we need it in this case. I also created #64547 so that we eventually remove these added mappings (if possible).

name: REINDEX_OP_TYPE,
hidden: false,
namespaceType: 'agnostic',
mappings: {
Copy link
Contributor

@pgayvallet pgayvallet Apr 27, 2020

Choose a reason for hiding this comment

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

If dynamic mappings in SO type are really a need, we can discuss about enabling it (actually it's only 'disabled' in the TS type, forcing the dynamic: true value does work, see (but please don't do):

// we don't want to allow `true` in the public `SavedObjectsTypeMappingDefinition` type, however
// this is needed for the config that is kinda a special type. To avoid adding additional internal types
// just for this, we hardcast to any here.
dynamic: true as any,

It's just that when we discussed about it, we did not find any valid argument to allow this value on the public API as a SO type mapping is meant to be exhaustive.

@cjcenizal
Copy link
Contributor Author

I tested UA by verifying that the sample telemetry data shown in Advanced Settings included UA data that reflected actions I've taken in the UI. I also stubbed out some server code to allow me to reindex sample data:

const getCombinedIndexInfos = (deprecations: DeprecationAPIResponse) => ([{
  index: 'kibana_sample_data_flights',
  reindex: true
}]);

This let me run the reindexing action in the UI and verify that it completed successfully.

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Changes look good to me, happy if CI is passing.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@cjcenizal cjcenizal merged commit 3b753ec into elastic:master Apr 28, 2020
@cjcenizal cjcenizal deleted the np/es-ui-cleanup branch April 28, 2020 16:41
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Apr 28, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 29, 2020
* master: (60 commits)
  [SIEM] Create template timeline (elastic#63136)
  load react component lazily in so management section (elastic#64285)
  Cleanup .eslingignore and add target (elastic#64617)
  [Ingest] Support yaml variables in datasource (elastic#64459)
  typescript-ify portions of src/optimize (elastic#64688)
  [ngSanitize] add explicit dependencies to all uses of `ngSanitize` angular module (elastic#64546)
  Consolidate downloading plugin bundles to bootstrap script (elastic#64685)
  [Maps] disable edit layer button when flyout is open for add layer or map settings (elastic#64230)
  chore(NA): add async import into infra plugin to reduce apm bundle size (elastic#63292)
  [Maps] fix edit filter (elastic#64586)
  [SIEM][Detections] Adds large list support using REST endpoints
  Replace a number of any-ed styled(eui*) with accurate types (elastic#64555)
  [Endpoint] Recursive resolver children (elastic#61914)
  [ML] Fix new job wizard with multiple indices (elastic#64567)
  Use short URLs for legacy plugin deprecation warning (elastic#64540)
  [Uptime] Update uptime ml job id to limit to 64 char (elastic#64394)
  [Ingest] Fix GET /enrollment-api-keys/null error (elastic#64595)
  Consolidate cross-cutting concerns between region & coordinate maps in new maps_legacy plugin (elastic#64123)
  ES UI new platform cleanup (elastic#64332)
  [Event Log] use @timestamp field for queries (elastic#64391)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:CCR and Remote Clusters Feature:Index Management Index and index templates UI Feature:NP Migration Feature:Upgrade Assistant release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants