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

NP Migration: Rollup plugin #53503

Merged
merged 54 commits into from
Jan 30, 2020
Merged

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Dec 18, 2019

This PR includes shimming of the Rollup plugin.

Early stages WIP PR:

  • actually call shimmed plugin
  • [ ] convert to typescript (moved to the Phase III)

Also here I got rid of injectI18n.

@elasticmachine

This comment has been minimized.

@jloleysens
Copy link
Contributor

@maryia-lapata

With regards to the issue: I am still able to reproduce it. I learned that this happens when specifying an index pattern like t* and a rollup index with a name like test. The error condition being that the rollup index will match itself.

The UI is specifically breaking because cause is not mapable. Looks like the cause variable is a string. @alisonelizabeth tested on master under the conditions described above and we should be seeing:

Screen Shot 2020-01-28 at 11 13 30 AM

So it does look like a regression introduced here. Let me know if you need more info!

id: 'rollup',

search: ({ searchRequests, Promise }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Tested locally and am happy with the proposed changes overall. Left some non-blocker comments.

@maryia-lapata
Copy link
Contributor

@jloleysens finally I reproduced the issue. Thanks for explanation. It was caused by a mistake in the code (here).
I fixed it.

How it looks now:
image

On the screenshot a status code is displayed as well. It was already envisaged here.

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

Didn't pull down to test.

@elastic/kibana-app-arch changes are 1 file

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@maryia-lapata maryia-lapata merged commit 0d03ade into elastic:master Jan 30, 2020
maryia-lapata added a commit that referenced this pull request Jan 30, 2020
* Start shimming rollup plugin

* continued shimming rollup ui

* Remove unnecessarily return

* Register management section

* Replace ui/chrome

* Replace ui/documentation_links

* Replace ui/kfetch and ui/courier

* Start shimming rollup plugin

* continued shimming rollup ui

* Remove unnecessarily return

* Register management section

* Replace ui/chrome

* Replace ui/documentation_links

* Replace ui/kfetch and ui/courier

* Replace ui/notify

* Move ui/ imports to legacy_imports.ts

* Update NP mock for management

* Refactoring

* Read body from error object

* Update setup_environment.js

* Update unit tests

* Get rid of injectI18n

* Replace npStart and npSetup usage to services

* Import search strategy stuff from the top level of the data plugin

* Update unit tests

* Do not prepend the url

* Fix merge conflicts

* Refactoring

* Revert removal of setUserHasLeftApp

* Export getSearchErrorType

* Remove extra wrapper - Router

* Fix cause prop.

* Leave just static imports in legacy_imports.js

* Add TS

* Pass statusCode instead of statusText

* Move template in a separate file

* Move app register to setup

* Add karma mock for management setup

* Add EditorConfigProviderRegistry export

Co-authored-by: Maryia Lapata <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Joe Reuter <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Feb 3, 2020
@maryia-lapata maryia-lapata added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Feb 10, 2020
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:NP Migration Feature:Rollups 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.

7 participants