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

back to canvas way of loading plugins #26463

Merged
merged 1 commit into from
Dec 5, 2018

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Nov 30, 2018

Summary

During moving interpreter to OSS dynamic plugin loading was broken, this tries to fix it, and goes back to the way canvas handled plugin loading before

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Member

@lukeelmers lukeelmers 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 (Chrome OSX), and loading custom plugins is working for me now.

Code LGTM too (other than the leftover console.log statements which need to be removed still), but we should get a sanity check from the Canvas team before merging.

@ppisljar ppisljar force-pushed the canvasPluginLoading branch from 20e4a5a to 482c447 Compare December 1, 2018 10:49
@ppisljar ppisljar added v7.0.0 Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Team:Visualizations Visualization editors, elastic-charts and infrastructure v6.6.0 labels Dec 1, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar ppisljar force-pushed the canvasPluginLoading branch from 484b34a to 3f323d9 Compare December 3, 2018 10:45
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar
Copy link
Member Author

ppisljar commented Dec 3, 2018

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar ppisljar force-pushed the canvasPluginLoading branch from 3f323d9 to 8dbdc5e Compare December 3, 2018 14:49
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ppisljar ppisljar added the review label Dec 3, 2018
Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the quick work on this.

import { getPluginStream } from '../lib/get_plugin_stream';

export function plugins(server) {
server.route({
method: 'GET',
path: '/api/canvas/plugins',
handler: function (request) {
handler: function (request, h) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make this a fully-stated variable, so it's clear what this is?

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, h is the standard name for the response toolkit in newer versions of Hapi

@cqliu1 cqliu1 mentioned this pull request Dec 3, 2018
@spalger spalger removed their request for review December 4, 2018 18:44
Copy link
Contributor

@rashidkpc rashidkpc left a comment

Choose a reason for hiding this comment

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

This LGTM, plugins seem to load ok and I know a couple other people that have made plugins work off of this.

@ppisljar ppisljar merged commit 83325bf into elastic:master Dec 5, 2018
ppisljar added a commit to ppisljar/kibana that referenced this pull request Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Team:Visualizations Visualization editors, elastic-charts and infrastructure v6.6.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants