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

Add a stub endpoint plugin #50082

Closed

Conversation

oatkiller
Copy link
Contributor

@oatkiller oatkiller commented Nov 7, 2019

Summary

This PR adds a basic (useless) endpoint plugin and Endpoint app. My assumption is that the Endpoint team will work merge their work into master. Up until the product is ready for release, the plugin will be disabled. When the app is release ready, we will make the plugin enabled by default and our commits will be added to a release branch.

TODO

  • The plugin is disabled by default
  • Should we try to follow the checklist below?

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@oatkiller oatkiller requested a review from a team as a code owner November 14, 2019 20:30
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

@elastic/kibana-platform could someone review/approve #50286 ?
I think we should separate core & plugin changes

"id": "endpoint",
"version": "1.0.0",
"kibanaVersion": "kibana",
"configPath": ["x-pack", "endpoint"],
Copy link
Contributor

Choose a reason for hiding this comment

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

--- x-pack
+++ xpack

async function greetingIndex(...passedArgs: [unknown, unknown, KibanaResponseFactory]) {
const [, , response] = passedArgs;
return response.ok({
body: JSON.stringify({ hello: 'world' }),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: don't think JSON.stringify is necessary

);
}

async function greetingIndex(...passedArgs: [unknown, unknown, KibanaResponseFactory]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use RequestHandler as a more descriptive type. We will remove the required types from the generic soon.

import { RequestHandler } from 'src/core/server';
export const greetingIndex: RequestHandler<any, any, any> = async (context, request, response) => {

schema: schema.object({ enabled: schema.boolean({ defaultValue: false }) }),
};

export function plugin() {
Copy link
Contributor

Choose a reason for hiding this comment

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

JFYI PluginInitializerContext also passed to the plugin factory

export const plugin = (initializerContext: PluginInitializerContext) => new EndpointPlugin(initializerContext)

import { schema } from '@kbn/config-schema';
import { EndpointPlugin } from './plugin';

export const config = {
Copy link
Contributor

Choose a reason for hiding this comment

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

config schema defined only on the server. lets remove it

export function addRoutes(router: IRouter) {
router.get(
{
path: '/endpoint/hello-world',
Copy link
Contributor

Choose a reason for hiding this comment

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

JFYI: by convention API endpoints are prefixed with api/

@mshustov
Copy link
Contributor

Should we try to follow the checklist below?

yes.

@oatkiller
Copy link
Contributor Author

@elastic/kibana-platform could someone review/approve #50286 ?
I think we should separate core & plugin changes

Sorry if I confused things. I was just pulling your commits in to see if they would fix my tests (they did.) I can close this PR until your PR is merged.

@oatkiller oatkiller closed this Nov 15, 2019
@mshustov
Copy link
Contributor

@oatkiller #50286 merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants