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

[SIEM] Client NP Cutover #64251

Merged
merged 39 commits into from
Apr 27, 2020
Merged

[SIEM] Client NP Cutover #64251

merged 39 commits into from
Apr 27, 2020

Conversation

rylnd
Copy link
Contributor

@rylnd rylnd commented Apr 22, 2020

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:

  • Moving of files
  • Logical changes
    • Declaration of client plugin dependencies in kibana.json
    • Removal of references to legacy folder
    • App registration (core.application.register)
      • this already existed, but I think the legacy app and links uiExports were overriding this behavior

TODO:

Reviewer Notes:

Checklist

For maintainers

rylnd added 26 commits April 22, 2020 12:22
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.
@rylnd rylnd added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels Apr 22, 2020
rylnd added 3 commits April 22, 2020 18:01
These were the legacy translations used... well, I don't know where they
were used.
@rylnd
Copy link
Contributor Author

rylnd commented Apr 23, 2020

RE IE11 testing: I can't get kibana to load on this branch due to the following errors:

Dashboard

However, these are also present on master so I guess we'll punt for now.

rylnd added 6 commits April 23, 2020 12:21
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.
@rylnd rylnd marked this pull request as ready for review April 27, 2020 15:38
@rylnd rylnd requested a review from a team as a code owner April 27, 2020 15:38
@rylnd rylnd requested a review from rudolf April 27, 2020 15:39
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';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rudolf I addressed this one specifically in dd1bcd6, but the initTelemetry and serviceNowActionType imports correspond to ~500kB and ~70kB worth of code, respectively. Without those, the plugin bundle is about 10kB (!). I created #64589 as a followup to this PR.

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.
Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

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.

image

 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
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@rylnd rylnd merged commit 4cc5b3a into elastic:master Apr 27, 2020
@rylnd rylnd deleted the rh/siem_client_np branch April 27, 2020 22:59
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.
@spong spong mentioned this pull request Apr 28, 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.
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants