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

Check for legacy imports in vis types and fix problems #56763

Merged
merged 10 commits into from
Feb 10, 2020

Conversation

flash1293
Copy link
Contributor

This PR turns on the ui/ import checks on all vis_type_* legacy plugins. Those should be completely shimmed already and no legacy utilities should be imported directly anymore.

However there were some validations, this PR fixes those as well. I'll leave some comments in the diff why certain things happened

@flash1293 flash1293 added release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.7.0 labels Feb 4, 2020
@@ -124,9 +123,6 @@ export const TEMPORARILY_IGNORED_PATHS = [
'src/legacy/core_plugins/timelion/server/series_functions/__tests__/fixtures/tlConfig.js',
'src/fixtures/config_upgrade_from_4.0.0_to_4.0.1-snapshot.json',
'src/legacy/core_plugins/vis_type_vislib/public/vislib/__tests__/lib/fixtures/mock_data/terms/_seriesMultiple.js',
'src/legacy/ui/public/angular-bootstrap/bindHtml/bindHtml.js',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

angular-bootstrap got moved into kibana_legacy - while doing this, I adjusted the naming conventions to our code where necessary.

@@ -88,6 +88,7 @@ import { createRenderCompleteDirective } from './np_ready/angular/directives/ren
* needs to render, so in the end the current 'kibana' angular module is no longer necessary
*/
export function getInnerAngularModule(name: string, core: CoreStart, deps: DiscoverStartPlugins) {
initAngularBootstrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the new way how to load angular-bootstrap modules. They still have to be listed as dependencies in the angular app modules.

@@ -21,6 +21,7 @@ import sinon from 'sinon';
import ngMock from 'ng_mock';
import expect from '@kbn/expect';

// eslint-disable-next-line @kbn/eslint/no-restricted-paths
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving in unallowed import here because these tests won't be migrated to the new platform anyway

const indexId = await this._findIndex(index);
const filter = esFilters.buildQueryFilter(query, indexId);

// This is a workaround for the https://github.com/elastic/kibana/issues/18863
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked and the filter gets removed and the chart gets re-rendered correctly. However there are several other things broken around vega and its filter management that makes it practically impossible to use the feature right now. I'll open a separate issue for those.

@flash1293 flash1293 marked this pull request as ready for review February 5, 2020 17:42
@flash1293 flash1293 requested a review from a team February 5, 2020 17:42
@flash1293 flash1293 requested a review from a team as a code owner February 5, 2020 17:42
@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Feb 5, 2020
@elasticmachine
Copy link
Contributor

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

Copy link
Member

@kertal kertal 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 in chrome. welcome to another citizen of kibana_legacy!

Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

The changes to files under operations team code owners LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@flash1293 flash1293 merged commit 09f1cad into elastic:master Feb 10, 2020
@flash1293 flash1293 deleted the vis_type_linting branch February 10, 2020 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants