-
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
Create stub endpoint plugin #50934
Create stub endpoint plugin #50934
Conversation
💔 Build Failed
|
💔 Build Failed
|
id: 'endpoint', | ||
title: 'Endpoint', | ||
async mount(context, params) { | ||
const { renderApp } = await import('./applications/endpoint'); |
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.
Should we name the chunk (for debugging purposes)?
const { renderApp } = await import(/* webpackChunkName: "endpoint" */ './applications/endpoint');
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.
@paul-tavares I'll have to defer to you. Can you provide me with a comment to add explaining?
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.
🙂
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.
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.
sounds good. just wanted that explanation to go along w/ it
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.
@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?
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.
@oatkiller is the import
here AMD-style?
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.
@bkimmel I believe it's https://github.com/tc39/proposal-dynamic-import
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.
@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:
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.
interestin
💚 Build Succeeded
|
💔 Build Failed
|
💔 Build Failed
|
dcd3aca
to
d50ce36
Compare
💔 Build Failed
|
return ( | ||
<I18nProvider> | ||
<h1 data-test-subj="welcomeMessage"> | ||
<FormattedMessage id="xpack.endpoint.welcomeMessage" defaultMessage="Hello World" /> |
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.
this is probably wrong?
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.
@oatkiller is this ID here for testing purposes? If so, you only need the top level data-test-subj
.
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.
This is for identifying the message during internationalization. You can read about them here:
- https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md
- https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/GUIDELINE.md
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', |
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.
is this right?
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.
@oatkiller this looks right. I'm testing it in my app now.
💔 Build Failed
|
5227419
to
f2c0d91
Compare
💔 Build Failed
|
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.
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" /> |
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.
@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.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
hash: '/management/elasticsearch/transform', | ||
}, | ||
endpoint: { | ||
pathname: '/app/endpoint', |
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.
@oatkiller this looks right. I'm testing it in my app now.
💔 Build Failed
|
@@ -0,0 +1,8 @@ | |||
{ | |||
"id": "endpoint", | |||
"version": "1.0.0", |
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.
should we call it a 1.0.0 or a 0.9.0 or does it matter in any way to us? @oatkiller
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.
I think this just reports a version number in the UI. I don't know who might rely on it. @james-elastic ?
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.
I don't think it matters either way but I filed #51226 to be sure.
💔 Build Failed
|
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.
First plugin app, with tests , FTW 🎉 🎉
x-pack/.i18nrc.json
Outdated
@@ -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" |
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.
it appears we've been trying, with some success (cough logstash cough), to keep this list in alphabetical order. move up?
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.
Is there a reason for keeping it in that order?
@@ -0,0 +1,8 @@ | |||
{ | |||
"id": "endpoint", | |||
"version": "1.0.0", |
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.
I don't think it matters either way but I filed #51226 to be sure.
|
||
ReactDOM.render(<AppRoot />, element); | ||
|
||
return function() { |
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.
return function() { | |
return () => { |
"Prefer arrow fns to fn expressions"
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.
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() { |
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.
const AppRoot = React.memo(function Root() { | |
const AppRoot = React.memo(() => { |
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.
Is there a reason to avoid naming functions?
@@ -0,0 +1,81 @@ | |||
{ |
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.
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); |
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.
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.
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.
@stacey-gammon thanks for the help.
public setup(core: CoreSetup) { | ||
core.application.register({ | ||
id: 'endpoint', | ||
title: 'Endpoint', |
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.
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',
}),
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.
Are proper-nouns like product and feature names usually translated?
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.
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'); |
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.
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.
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.
thanks. i switched it for the sake of making my intent clear
982d829
to
a56646d
Compare
id: 'endpoint', | ||
name: 'Endpoint', | ||
icon: 'bug', | ||
navLinkId: 'endpoint', |
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.
@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:
and then creating a user that has only that role:
Logging in as this user shows the 'E' link in the nav for the Endpoint app:
The user cannot navigate to the app (as expected):
@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!
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.
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?
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.
@joshdover Yes that's right. (but I'll confirm just to be sure)
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.
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 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.
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.
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.
💔 Build Failed
|
💔 Build Failed
|
@elasticmachine merge upstream |
05c7ab7
to
7a7d731
Compare
💔 Build Failed
|
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
Any ideas? |
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 Going to put everything back and run with just the app registration removed. will report back! |
Taking a look. It is possible there's an issue with production builds of dynamic imports. |
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. |
@oatkiller You should be able to merge in master now to resolve these X-Pack failures due to the bundling issue. |
28a5eea
to
0d34dac
Compare
💔 Build Failed |
FYI, #51438 has been merged, you can reintegrate these changes from master now. |
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:
And in IE11
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Documentation was added for features that require explanation or tutorials- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers