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

Create stub endpoint plugin #50934

Closed
wants to merge 24 commits into from

Conversation

oatkiller
Copy link
Contributor

@oatkiller oatkiller commented Nov 18, 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 reddy, we will make the plugin enabled by default and our commits will be added to a release branch.

The new Endpoint app, showing 'Hello World' text and a new 'E' top-level application icon:
The new Endpoint app, showing 'Hello World' text and a new 'E' top-level application icon

And in IE11
image

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

id: 'endpoint',
title: 'Endpoint',
async mount(context, params) {
const { renderApp } = await import('./applications/endpoint');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we name the chunk (for debugging purposes)?

const { renderApp } = await import(/* webpackChunkName: "endpoint" */ './applications/endpoint');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paul-tavares I'll have to defer to you. Can you provide me with a comment to add explaining?

Copy link
Contributor

Choose a reason for hiding this comment

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

🙂
The code block-comment to add is above (sorry - should have made that clear). Essentially, add this before the file path to load: /* webpackChunkName: 'endpoint' */' . This instructs webpack on the name to use for the output bundle instead of the default set of characters.

It's not a must, but helpful if one is looking at the browser network tab to quickly identify the chunk that holds our plugin's code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good. just wanted that explanation to go along w/ it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paul-tavares seems like its 'endpoint.bundle.js' (at least in dev) by default. Is that the thing you're expecting to change? What would you change it to?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

@oatkiller is the import here AMD-style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@oatkiller Just checked my POC - you right. The initial (plugin entry point) bundle created does in fact inherit the naming of the plugin. Sorry about that.

I think my comment would apply to any dynamic loading done after the initial plugin bundle. Example: in my POC, I did code splitting at each "primary" view - so Landing page, Endpoints, Alerts, etc. For those dynamic imports, if you don't given it a name, they will be created with webpack's default.

Example:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interestin

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@oatkiller oatkiller force-pushed the create-stub-endpoint-plugin branch 2 times, most recently from dcd3aca to d50ce36 Compare November 19, 2019 14:04
@elasticmachine
Copy link
Contributor

💔 Build Failed

return (
<I18nProvider>
<h1 data-test-subj="welcomeMessage">
<FormattedMessage id="xpack.endpoint.welcomeMessage" defaultMessage="Hello World" />
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 probably wrong?

Choose a reason for hiding this comment

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

@oatkiller is this ID here for testing purposes? If so, you only need the top level data-test-subj.

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 for identifying the message during internationalization. You can read about them here:

The second document 'GUIDELINE.md' talks about naming conventions a bit. I can't follow it just yet.

hash: '/management/elasticsearch/transform',
},
endpoint: {
pathname: '/app/endpoint',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this right?

Choose a reason for hiding this comment

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

@oatkiller this looks right. I'm testing it in my app now.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@oatkiller oatkiller force-pushed the create-stub-endpoint-plugin branch from 5227419 to f2c0d91 Compare November 19, 2019 18:07
@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link

@aisantos aisantos left a comment

Choose a reason for hiding this comment

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

Looks good overall, except can you delete the tags in the API integration level tests?

return (
<I18nProvider>
<h1 data-test-subj="welcomeMessage">
<FormattedMessage id="xpack.endpoint.welcomeMessage" defaultMessage="Hello World" />

Choose a reason for hiding this comment

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

@oatkiller is this ID here for testing purposes? If so, you only need the top level data-test-subj.

export default function endpointAPIIntegrationTests({ loadTestFile }: FtrProviderContext) {
describe('Endpoint', function() {
this.tags('ciGroup7');
loadTestFile(require.resolve('./resolver'));

This comment was marked as resolved.

This comment was marked as resolved.

hash: '/management/elasticsearch/transform',
},
endpoint: {
pathname: '/app/endpoint',

Choose a reason for hiding this comment

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

@oatkiller this looks right. I'm testing it in my app now.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@@ -0,0 +1,8 @@
{
"id": "endpoint",
"version": "1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

should we call it a 1.0.0 or a 0.9.0 or does it matter in any way to us? @oatkiller

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 think this just reports a version number in the UI. I don't know who might rely on it. @james-elastic ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it matters either way but I filed #51226 to be sure.

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

First plugin app, with tests , FTW 🎉 🎉

@@ -38,7 +38,8 @@
"xpack.transform": "legacy/plugins/transform",
"xpack.upgradeAssistant": "legacy/plugins/upgrade_assistant",
"xpack.uptime": "legacy/plugins/uptime",
"xpack.watcher": "legacy/plugins/watcher"
"xpack.watcher": "legacy/plugins/watcher",
"xpack.endpoint": "plugins/endpoint"
Copy link
Contributor

Choose a reason for hiding this comment

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

it appears we've been trying, with some success (cough logstash cough), to keep this list in alphabetical order. move up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason for keeping it in that order?

@@ -0,0 +1,8 @@
{
"id": "endpoint",
"version": "1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it matters either way but I filed #51226 to be sure.


ReactDOM.render(<AppRoot />, element);

return function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return function() {
return () => {

nit: https://github.com/elastic/kibana/blob/master/STYLEGUIDE.md#prefer-modern-javascripttypescript-syntax

"Prefer arrow fns to fn expressions"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

curious what the reasoning is there. Seems to hinder readability (harder to scan the text and see 'function'.) It also leaves the reader wondering if there is a reason for the arrow function, in other words, has this been used.

};
}

const AppRoot = React.memo(function Root() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const AppRoot = React.memo(function Root() {
const AppRoot = React.memo(() => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason to avoid naming functions?

@@ -0,0 +1,81 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need this file or folder.

export function renderApp(appMountContext: AppMountContext, { element }: AppMountParameters) {
appMountContext.core.http.get('/api/endpoint/hello-world');

ReactDOM.render(<AppRoot />, element);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, for internationalization in a NP app, you want:

export function renderApp(appMountContext: AppMountContext, { element }: AppMountParameters) {
  appMountContext.core.http.get('/api/endpoint/hello-world');

  ReactDOM.render(
    <appMountContext.core.i18n.Context>
      <AppRoot />
    </appMountContext.core.i18n.Context>,
    element
  );

  return function() {
    ReactDOM.unmountComponentAtNode(element);
  };
}

const AppRoot = React.memo(function Root() {
  return (
    <h1 data-test-subj="welcomeTitle">
      <FormattedMessage id="xpack.endpoint.welcomeTitle" defaultMessage="Hello World" />
    </h1>
  );
});

cc @Bamieh -- is that correct? https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#internationalization recommends I18nContext but it's showing the legacy way of importing. This also seems to contradict the README which shows I18nProvider still... and the Guideline doesn't mention either term.

Maybe hold off on changing, @oatkiller, until we get confirmation on the right way to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stacey-gammon thanks for the help.

public setup(core: CoreSetup) {
core.application.register({
id: 'endpoint',
title: 'Endpoint',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we probably want this translated as well. I notice the Discover title is


import { i18n } from '@kbn/i18n';
... 
        title: i18n.translate('kbn.discoverTitle', {
            defaultMessage: 'Discover',
          }),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are proper-nouns like product and feature names usually translated?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think they should be. I googled "Pepsi" in Chinese and something comes up, and some random website says

Route 1: Keep in the source language
As a general rule, if the source script and target script are different (Arabic to English, or Thai to English, for example), no words should be left in the source script in your translation"

As far as company policy though, I don't know. I would guess over translating is better than under translating. cc @Bamieh - if you have any additional insight.

});

it("welcomes the user with 'Hello World'", async function() {
await testSubjects.find('welcomeTitle');
Copy link
Contributor

Choose a reason for hiding this comment

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

just fyi, can also use await testSubjects.existOrFail('welcomeTitle'), but I think that is logically equivalent to what you are doing anyway, so, feel free to leave as is as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. i switched it for the sake of making my intent clear

@oatkiller oatkiller force-pushed the create-stub-endpoint-plugin branch from 982d829 to a56646d Compare November 21, 2019 16:51
id: 'endpoint',
name: 'Endpoint',
icon: 'bug',
navLinkId: 'endpoint',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stacey-gammon @joshdover
Endpoint had no registerFeature call and as a result was showing up regardless of the user's role. I read some docs and took a guess by adding this. The icon still shows up even when I login as a user that shouldn't have access to Endpoint.

I did a test by creating a role that should not have access to Endpoint:
image
and then creating a user that has only that role:
image

Logging in as this user shows the 'E' link in the nav for the Endpoint app:
image

The user cannot navigate to the app (as expected):
image

@joshdover Suggested that the link-hiding functionality of this system is not yet available in new-platform plugins. He recommended creating a legacy plugin to complement our new platform one.

@stacey-gammon @joshdover Could I have some help with this? Is this the right direction? How would I do it? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this isn't implemented yet for New Platform applications, and I don't think there's a great way to workaround this right now. We're implementing this feature right now.

To be clear, when the plugin is disabled the icon is hidden, correct? It's just not hidden when the plugin is enabled and the user doesn't have access to Endpoint via their privileges?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshdover Yes that's right. (but I'll confirm just to be sure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plugin is disabled by default and the link is not there by default:
image
image
So I'd say that's working fine. Sorry for the confusion

Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably then just punt feature controls to a different PR. There really is nothing about this app (esp as it is now "hello world") that would require feature controls in a first iteration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I agree. By the time we'll be putting this in front of users, the Platform changes will be in place to support feature controls here, without any changes required by the Endpoint app.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor

@elasticmachine merge upstream

@oatkiller oatkiller force-pushed the create-stub-endpoint-plugin branch from 05c7ab7 to 7a7d731 Compare November 21, 2019 21:43
@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor

Does anyone from @elastic/kibana-operations team have a clue what could be going on here? This PR looks super simple, yet it fails ci with Failed to load "endpoint" bundle (/bundles/plugin/endpoint.bundle.js), but only when run against the build, it passes when run with separate server vs runner commands.

                 │          at HTMLScriptElement.script.onerror (http://localhost:5620/bundles/commons.bundle.js:3:2421743)
                 │ debg browser[INFO] http://localhost:5620/bundles/commons.bundle.js 2:2324143 "Detected an unhandled Promise rejection.
                 │      Error: Failed to load \"endpoint\" bundle (/bundles/plugin/endpoint.bundle.js)"
                 │ERROR browser[SEVERE] http://localhost:5620/bundles/commons.bundle.js 2:2421742 Uncaught Error: Failed to load "endpoint" bundle (/bundles/plugin/endpoint.bundle.js)
                 │ debg --- retry.try error: Waiting for element to be located By(css selector, [data-test-subj="kibanaChrome"])
                 │      Wait timed out after 61173ms

Any ideas?

@stacey-gammon
Copy link
Contributor

fwiw it passes locally when I completely took out the ui component from the app. Which, I expected, but, there it is. I think this is the first new platform plugin with a registered ui app that isn't part of plugin_functional. Maybe that has something to do with it.

Going to put everything back and run with just the app registration removed. will report back!

@joshdover
Copy link
Contributor

Taking a look. It is possible there's an issue with production builds of dynamic imports.

@joshdover
Copy link
Contributor

I believe this is happening because the optimizer does not build the plugin bundle for plugins that are disabled. So when Kibana is started with the endpoint plugin enabled, the bundle is not present and it fails to load.

Working on reproducing this now. If this is the case, I'll open an issue so we can figure out what this behavior should be.

@joshdover
Copy link
Contributor

@oatkiller You should be able to merge in master now to resolve these X-Pack failures due to the bundling issue.

@oatkiller oatkiller force-pushed the create-stub-endpoint-plugin branch from 28a5eea to 0d34dac Compare December 2, 2019 13:17
@elasticmachine
Copy link
Contributor

💔 Build Failed

@pgayvallet
Copy link
Contributor

FYI, #51438 has been merged, you can reintegrate these changes from master now.

@oatkiller oatkiller closed this Dec 17, 2019
@oatkiller oatkiller deleted the create-stub-endpoint-plugin branch January 15, 2020 18:50
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.

None yet

9 participants