Merged
Conversation
This is solely renames; fixes come next.
These paths got a little shorter, so some lines could be collapsed.
I'm removing the @types/js-yaml for now because I'm not sure we need it; I'll add it back later if we do.
This previously had to be part of legacy bootstrapping due to an order of operations issue.
The index file should now be redundant with what's in the plugin: * app registration * feature registration
We can now use the NP API. Maps embeddable will not work here until their work is merged, but this should prevent us from importing legacy code and thus breaking the build.
These are all required for now, because that's how they're typed. If they _should_ be optional (and I think several should), we need to update the type and handle the null case within the app.
We're not using their contracts, but we are importing code from them.
* Replace build-breaking ui/new_platform mocks with equivalents in core proper * Remove unnecessary mocks of ui/new_platform
* I left the reference in CODEOWNERS in case someone tries to sneak something back * I left the .gitignore reference for the same reason
These were not caught by typescript and were causing test failures.
They're empty for now.
The new one dropped a param we weren't using.
Import them from core where we need them, instead
This is already imported, there's no benefit (and potential timing issues) with doing this inside the mount.
This doesn't change what's used, only how we're typing it. The types are now a little more truthful as: * our StartPlugins don't include setup contracts * our StartServices includes everything we use at Start time, including the one setup plugin.
These are shared, and should be consistent.
These were the legacy translations used... well, I don't know where they were used.
rylnd
commented
Apr 23, 2020
Contributor
Author
|
RE IE11 testing: I can't get kibana to load on this branch due to the following errors:
However, these are also present on master so I guess we'll punt for now. |
We import our maps embeddable from maps, and we pass inspector to the embeddable. I just missed these in my audit. This was causing errors in the map embeddable.
This is an async call that we need to complete before we can render.
Conflicts: x-pack/index.js
rudolf
reviewed
Apr 27, 2020
| import { SecurityPluginSetup } from '../../security/public'; | ||
| import { APP_ID, APP_NAME, APP_PATH, APP_ICON } from '../common/constants'; | ||
| import { initTelemetry } from './lib/telemetry'; | ||
| import { KibanaServices } from './lib/kibana'; |
Contributor
There was a problem hiding this comment.
Because we don't do tree-shaking this will inadvertently pull in the kibana_react plugin. It's currently an external module so this won't increase the bundle-size, but it would be better to have separate modules for KibanaServices used in the plugin.ts and the kibana_react and other dependencies used in app.tsx
Contributor
Author
There was a problem hiding this comment.
When we load our initial plugin (before our app is loaded), were were implicitly importing all of kibana_react with this import. While a global module prevents this from affecting our bundle size currently, that could change in the future. Since we only need a reference to our class, we just import that instead.
XavierM
approved these changes
Apr 27, 2020
Contributor
XavierM
left a comment
There was a problem hiding this comment.
LGTM,
Tested Locally all the different pages and functionalities on chrome, everything is working as expected.
The code change makes sense and the Map is working <3.
Conflicts: Jenkinsfile x-pack/plugins/siem/public/containers/case/configure/mock.ts x-pack/plugins/siem/public/pages/case/components/configure_cases/index.test.tsx x-pack/plugins/siem/public/pages/case/components/configure_cases/reducer.test.ts x-pack/plugins/siem/public/pages/case/components/configure_cases/reducer.ts
Contributor
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
rylnd
added a commit
to rylnd/kibana
that referenced
this pull request
Apr 27, 2020
* Move SIEM public/ folder to NP plugin This is solely renames; fixes come next. * Update relative imports in our API tests * Fix linter errors following move to NP folder These paths got a little shorter, so some lines could be collapsed. * Move client dependencies to NP package.json I'm removing the @types/js-yaml for now because I'm not sure we need it; I'll add it back later if we do. * Fix relative imports to other plugins * Fix errant uses of ui/chrome * Remove legacy plugin shim * Move feature registration into plugin This previously had to be part of legacy bootstrapping due to an order of operations issue. * Disconnect legacy plugin The index file should now be redundant with what's in the plugin: * app registration * feature registration * Move public gitattributes * Remove references to legacy embeddables We can now use the NP API. Maps embeddable will not work here until their work is merged, but this should prevent us from importing legacy code and thus breaking the build. * Add our frontend dependencies to kibana.json These are all required for now, because that's how they're typed. If they _should_ be optional (and I think several should), we need to update the type and handle the null case within the app. * Replace use of ui/new_platform mocks in embeddable utils * Fix remaining jest tests * Replace build-breaking ui/new_platform mocks with equivalents in core proper * Remove unnecessary mocks of ui/new_platform * Remove references to legacy SIEM folder * I left the reference in CODEOWNERS in case someone tries to sneak something back * I left the .gitignore reference for the same reason * Fix mocks of relative paths These were not caught by typescript and were causing test failures. * Export our client plugin contracts They're empty for now. * Move from deprecated appmount API The new one dropped a param we weren't using. * Add missing mock causing test failures * Don't re-export core types from our plugin Import them from core where we need them, instead * Move Actions UI registry outside of mount This is already imported, there's no benefit (and potential timing issues) with doing this inside the mount. * Add security's setup contract to our StartServices This doesn't change what's used, only how we're typing it. The types are now a little more truthful as: * our StartPlugins don't include setup contracts * our StartServices includes everything we use at Start time, including the one setup plugin. * Add order and icon back to the sidebar link * Replace plugin class properties with constants These are shared, and should be consistent. * Enable our UI on NP * Add missed plugin dependencies We're not using their contracts, but we are importing code from them. * Revert use of constant in translation Can't do that, whoops * i18n our feature catalogue entry * Remove unnecessary array from single element * Remove unused keys These were the legacy translations used... well, I don't know where they were used. * Ignore circular dependencies in external plugins * Normalize exclusions * Add undeclared dependencies to kibana.json We import our maps embeddable from maps, and we pass inspector to the embeddable. I just missed these in my audit. This was causing errors in the map embeddable. * Await our call to setLayerList This is an async call that we need to complete before we can render. * Reduce siem plugin size When we load our initial plugin (before our app is loaded), were were implicitly importing all of kibana_react with this import. While a global module prevents this from affecting our bundle size currently, that could change in the future. Since we only need a reference to our class, we just import that instead.
This was referenced Apr 27, 2020
26 tasks
rylnd
added a commit
that referenced
this pull request
Apr 28, 2020
* Move SIEM public/ folder to NP plugin This is solely renames; fixes come next. * Update relative imports in our API tests * Fix linter errors following move to NP folder These paths got a little shorter, so some lines could be collapsed. * Move client dependencies to NP package.json I'm removing the @types/js-yaml for now because I'm not sure we need it; I'll add it back later if we do. * Fix relative imports to other plugins * Fix errant uses of ui/chrome * Remove legacy plugin shim * Move feature registration into plugin This previously had to be part of legacy bootstrapping due to an order of operations issue. * Disconnect legacy plugin The index file should now be redundant with what's in the plugin: * app registration * feature registration * Move public gitattributes * Remove references to legacy embeddables We can now use the NP API. Maps embeddable will not work here until their work is merged, but this should prevent us from importing legacy code and thus breaking the build. * Add our frontend dependencies to kibana.json These are all required for now, because that's how they're typed. If they _should_ be optional (and I think several should), we need to update the type and handle the null case within the app. * Replace use of ui/new_platform mocks in embeddable utils * Fix remaining jest tests * Replace build-breaking ui/new_platform mocks with equivalents in core proper * Remove unnecessary mocks of ui/new_platform * Remove references to legacy SIEM folder * I left the reference in CODEOWNERS in case someone tries to sneak something back * I left the .gitignore reference for the same reason * Fix mocks of relative paths These were not caught by typescript and were causing test failures. * Export our client plugin contracts They're empty for now. * Move from deprecated appmount API The new one dropped a param we weren't using. * Add missing mock causing test failures * Don't re-export core types from our plugin Import them from core where we need them, instead * Move Actions UI registry outside of mount This is already imported, there's no benefit (and potential timing issues) with doing this inside the mount. * Add security's setup contract to our StartServices This doesn't change what's used, only how we're typing it. The types are now a little more truthful as: * our StartPlugins don't include setup contracts * our StartServices includes everything we use at Start time, including the one setup plugin. * Add order and icon back to the sidebar link * Replace plugin class properties with constants These are shared, and should be consistent. * Enable our UI on NP * Add missed plugin dependencies We're not using their contracts, but we are importing code from them. * Revert use of constant in translation Can't do that, whoops * i18n our feature catalogue entry * Remove unnecessary array from single element * Remove unused keys These were the legacy translations used... well, I don't know where they were used. * Ignore circular dependencies in external plugins * Normalize exclusions * Add undeclared dependencies to kibana.json We import our maps embeddable from maps, and we pass inspector to the embeddable. I just missed these in my audit. This was causing errors in the map embeddable. * Await our call to setLayerList This is an async call that we need to complete before we can render. * Reduce siem plugin size When we load our initial plugin (before our app is loaded), were were implicitly importing all of kibana_react with this import. While a global module prevents this from affecting our bundle size currently, that could change in the future. Since we only need a reference to our class, we just import that instead.
Contributor
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is meant to be the final PR for Siem's NP Migration (#45831). Similar to its companion (#63430), I've chunked this into two logical pieces:
core.application.register)appandlinksuiExports were overriding this behaviorTODO:
That PR has been merged to master (and here), but our map still isn't rendering. Currently investigating.Fixed in [Maps] Include maps styles in embeddables factory so that they're bundled w/ consuming plugins #64460Reviewer Notes:
@types/js-yamlfrom our local package.json as it seemed like dead code (our types are working)react-beautiful-dndwas removed from our package.json in Upgrade to EUI v22.3.0 #62963. I haven't seen any issues there, but it's worth mentioning again.Checklist
For maintainers