-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Canvas] Move workpad api routes to New Platform #51116
Conversation
Pinging @elastic/kibana-canvas (Team:Canvas) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you already have a server-side shim, any reason not to keep these routes in that shimmed plugin (but still using the New Platform router)?
By keeping it in the shimmed legacy plugin you'll be able to move the entire legacy server code over at once which may make the final migration step a bit easier.
} | ||
|
||
public stop() { | ||
this.initializerContext.logger.get().debug('Stopping Canvas plugin'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, Core already logs messages for each plugin lifecycle method, so these may be superfluous.
*/ | ||
|
||
import sinon from 'sinon'; | ||
import Sinon from 'sinon'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two sinons?
statusCode: error.output.statusCode, | ||
}); | ||
} | ||
return response.badRequest({ body: error }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's an unexpected error, I think we should use response.internalError
for a 500 error.
*/ | ||
|
||
import sinon from 'sinon'; | ||
import Sinon from 'sinon'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More double sinons :)
/** | ||
* Represents Code Plugin instance that will be managed by the Kibana plugin system. | ||
*/ | ||
export class CanvasPlugin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should import and implement the Plugin
interface from src/core/server
here.
💚 Build Succeeded |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm -- tested on sample workpad data and it worked great
💚 Build Succeeded |
* Move workpad api routes to New Platform * Cleanup * Clean up/Pr Feedback * Adding missing dependency to tests * Fix typecheck * Loosen workpad schema restrictions
Summary
This PR creates the Canvas plugin that will be loaded via the New Platform.
Also adds all of the routes for the workpads api to this new plugin and removes them from the canvas legacy plugin. Important note here is this also adds validation to all of the params or bodys of those requests, so please test whatever ways you can think of that these routes might be called to ensure they are never called in an incorrect way with params that will fail validation.
Summarize your PR. If it involves visual changes include a screenshot or gif.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers