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

[Fleet] Move integrations to a separate app #99848

Merged
merged 37 commits into from
Jun 7, 2021

Conversation

kpollich
Copy link
Member

@kpollich kpollich commented May 11, 2021

This is the initial work required to move the Integrations UI out into its own Fleet app. The goal here is to allow us to make room for substantially expanding and rethinking what Integrations are and what they do.

A large chunk of this work came from referencing @jen-huang's POC branch from a few months ago: https://github.com/jen-huang/kibana/tree/poc/integrations-ui.

This commit moves the sections/epm directory from the Fleet application to a new Integrations application, still within the Fleet codebase. This should be considered the first step, and it's likely that we'll want to consider moving Integrations to its own plugin entirely at a later date.

This PR addresses the following tasks in the parent tracking issue:

  • Move Integrations section of Fleet app to new Integrations app, linked under Management in Kibana nav
  • Adjust routing, breadcrumbs, and related url/routing code
  • Adjust Integrations layout to have 2 top-level tabs: Browse, Manage
  • Adjust Fleet layout to have 4 top-level tabs: Agents, Agent policies, Enrollment tokens, Data streams (Assuming this one is outdated - the new tabs are Overview, Policies, Agents, Data Streams)

Screenshot Comparison

Screen Main Branch PR Notes
Fleet Home fleet-home fleet-home Integrations removed from navbar, "View integrations" links to Integrations app
Integrations Browse integrations-home integrations-home Moved to Integrations App
Integrations Installed integrations-installed integrations-installed Moved to Integrations App
Integration Details - Nginx integrations-details-nginx integrations-details-nginx Moved to Integrations App, breadcrumbs use package name
Add Integrations - Nginx add-integration-nginx add-integration-nginx Remains in Fleet app. Breadcrumbs weren't easily preserved. May need a follow up to address
Kibana Sidebar kibana-sidebar kibana-sidebar Added Integrations link above Fleet per designs

@kpollich kpollich added v8.0.0 Team:Fleet Team label for Observability Data Collection Fleet team v7.14.0 auto-backport Deprecated: Automatically backport this PR after it's merged labels May 11, 2021
@kpollich
Copy link
Member Author

kpollich commented May 14, 2021

Edit: Fixed the tests mentioned here

These tests are now fixed

Blocked on getting some tests to pass here now that they've moved:

x-pack/plugins/fleet/public/applications/integrations/sections/epm/screens/detail/index.test.tsx

Tests for the "policies tab" are failing, I think because of a routing issue where the <PackagePoliciesPage /> component isn't being rendered.

when on integration detail › and on the Policies Tab › should display policies list

    TestingLibraryElementError: Unable to find an element by: [data-test-subj="integrationPolicyTable"]

    <body>
      <div />
    </body>

      205 |     it('should display policies list', async () => {
      206 |       await mockedApi.waitForApi();
    > 207 |       const table = renderResult.getByTestId('integrationPolicyTable');
          |                                  ^
      208 |       expect(table).not.toBeNull();
      209 |     });
      210 |

      at Object.getElementError (node_modules/@testing-library/dom/dist/config.js:37:19)
      at node_modules/@testing-library/dom/dist/query-helpers.js:90:38
      at node_modules/@testing-library/dom/dist/query-helpers.js:62:17
      at getByTestId (node_modules/@testing-library/dom/dist/query-helpers.js:111:19)
      at Object.<anonymous> (x-pack/plugins/fleet/public/applications/integrations/sections/epm/screens/detail/index.test.tsx:207:34)

  ● when on integration detail › and on the Policies Tab › should link to integration policy detail when an integration policy is clicked

    TestingLibraryElementError: Unable to find an element by: [data-test-subj="integrationNameLink"]

    <body>
      <div />
    </body>

      211 |     it('should link to integration policy detail when an integration policy is clicked', async () => {
      212 |       await mockedApi.waitForApi();
    > 213 |       const firstPolicy = renderResult.getAllByTestId(
          |                                        ^
      214 |         'integrationNameLink'
      215 |       )[0] as HTMLAnchorElement;
      216 |       expect(firstPolicy.href).toEqual(

      at Object.getElementError (node_modules/@testing-library/dom/dist/config.js:37:19)
      at node_modules/@testing-library/dom/dist/query-helpers.js:90:38
      at getAllByTestId (node_modules/@testing-library/dom/dist/query-helpers.js:130:15)
      at Object.<anonymous> (x-pack/plugins/fleet/public/applications/integrations/sections/epm/screens/detail/index.test.tsx:213:40)

  ● when on integration detail › and on the Policies Tab › should NOT show link for agent count if it is zero

    TestingLibraryElementError: Unable to find an element by: [data-test-subj="rowAgentCount"]

    <body>
      <div />
    </body>

      221 |     it('should NOT show link for agent count if it is zero', async () => {
      222 |       await mockedApi.waitForApi();
    > 223 |       const firstRowAgentCount = renderResult.getAllByTestId('rowAgentCount')[0];
          |                                               ^
      224 |       expect(firstRowAgentCount.textContent).toEqual('0');
      225 |       expect(firstRowAgentCount.tagName).not.toEqual('A');
      226 |     });

      at Object.getElementError (node_modules/@testing-library/dom/dist/config.js:37:19)
      at node_modules/@testing-library/dom/dist/query-helpers.js:90:38
      at getAllByTestId (node_modules/@testing-library/dom/dist/query-helpers.js:130:15)
      at Object.<anonymous> (x-pack/plugins/fleet/public/applications/integrations/sections/epm/screens/detail/index.test.tsx:223:47)

  ● when on integration detail › and on the Policies Tab › should show link for agent count if greater than zero

    TestingLibraryElementError: Unable to find an element by: [data-test-subj="rowAgentCount"]

    <body>
      <div />
    </body>

      228 |     it('should show link for agent count if greater than zero', async () => {
      229 |       await mockedApi.waitForApi();
    > 230 |       const secondRowAgentCount = renderResult.getAllByTestId('rowAgentCount')[1];
          |                                                ^
      231 |       expect(secondRowAgentCount.textContent).toEqual('100');
      232 |       expect(secondRowAgentCount.tagName).toEqual('A');
      233 |     });

      at Object.getElementError (node_modules/@testing-library/dom/dist/config.js:37:19)
      at node_modules/@testing-library/dom/dist/query-helpers.js:90:38
      at getAllByTestId (node_modules/@testing-library/dom/dist/query-helpers.js:130:15)
      at Object.<anonymous> (x-pack/plugins/fleet/public/applications/integrations/sections/epm/screens/detail/index.test.tsx:230:48)

There are also a ton of React Testing Library warnings around act calls for state updates that I'm not sure how to resolve. The errors aren't super clear, but they're repeated for almost every test:

console.error
    Warning: An update to FleetStatusProvider inside a test was not wrapped in act(...).

    When testing, code that causes React state updates should be wrapped into act(...):

    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */

    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act
        in FleetStatusProvider
        in ThemeProvider (created by EuiThemeProvider)
        in EuiThemeProvider
        in EuiErrorBoundary
        in Provider
        in PseudoLocaleWrapper (created by I18nProvider)
        in IntlProvider (created by I18nProvider)
        in I18nProvider (created by mockConstructor)
        in mockConstructor
        in Unknown
        in Unknown

      42 |       }
      43 |
    > 44 |       setState((s) => ({
         |       ^
      45 |         ...s,
      46 |         isLoading: false,
      47 |         isReady: res.data?.isReady ?? false,

      at warningWithoutStack (node_modules/react-dom/cjs/react-dom.development.js:530:32)
      at warnIfNotCurrentlyActingUpdatesInDEV (node_modules/react-dom/cjs/react-dom.development.js:25848:7)
      at setState (node_modules/react-dom/cjs/react-dom.development.js:17072:9)
      at sendGetStatus (x-pack/plugins/fleet/public/hooks/use_fleet_status.tsx:44:7)

@kpollich kpollich force-pushed the integrations-separate-app branch 2 times, most recently from e18ca4b to d48b017 Compare May 31, 2021 23:08
@kpollich kpollich self-assigned this Jun 1, 2021
@jloleysens jloleysens self-requested a review June 1, 2021 15:04
@kpollich kpollich marked this pull request as ready for review June 1, 2021 15:13
@kpollich kpollich requested a review from a team as a code owner June 1, 2021 15:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@kpollich kpollich changed the title [Fleet] [WIP] Move integrations to a separate app [Fleet] Move integrations to a separate app Jun 1, 2021
@kpollich
Copy link
Member Author

kpollich commented Jun 1, 2021

Edit: This was resolved in a310e4a

Getting an odd test failure that I could use a second set of eyes on in the latest here (appears in a few tests) - seems like some kind of issue with the context components. For some reason this UIExtensionsContext object is coming back undefined in tests. Other similar providers like KibanaVersionContext or ConfigContext work as expected. Issue seems unique to UIExtensionsContext.

when on the package policy create page › and Route state is provided via Fleet HashRouter › and the cancel Link or Button is clicked › should redirect via Fleet HashRouter when cancel Button (button bar) is clicked

    TypeError: Cannot read property 'Provider' of undefined

      213 |               <KibanaVersionContext.Provider value={kibanaVersion}>
      214 |                 <EuiThemeProvider darkMode={isDarkMode}>
    > 215 |                   <UIExtensionsContext.Provider value={extensions}>
          |                                        ^
      216 |                     <FleetStatusProvider>
      217 |                       <IntraAppStateProvider kibanaScopedHistory={history}>
      218 |                         <Router history={routerHistoryInstance}>

@@ -6,3 +6,4 @@
*/

export const PLUGIN_ID = 'fleet';
export const INTEGRATIONS_PLUGIN_ID = 'integrations';
Copy link
Member

Choose a reason for hiding this comment

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

Do we plan to move integrations to a different plugin? or it will always be an app inside the fleet plugin?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the plan would be to eventually move it to a separate plugin entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

later on it may become separate, but that's not firmly decided at this point

@jen-huang jen-huang added the release_note:feature Makes this part of the condensed release notes label Jun 1, 2021
@kpollich kpollich requested a review from a team as a code owner June 1, 2021 20:30
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Great work so far @kpollich !

I left a few comments in the code, not any blockers for now.

I tested locally and saw the following behaviour when view an integration package:

Screenshot 2021-06-02 at 12 09 44

It looks like this is one of the markdown renderers (https://github.com/kpollich/kibana/blob/b174e639dab53190c1393f1f7519cd9b259ac46d/x-pack/plugins/fleet/public/applications/integrations/sections/epm/screens/detail/overview/markdown_renderers.tsx#L62) receiving an unknown language $json. Not sure whether this is a known problem?

@jloleysens
Copy link
Contributor

Getting an odd test failure that I could use a second set of eyes on in the latest here (appears in a few tests) - seems like some kind of issue with the context components. For some reason this UIExtensionsContext object is coming back undefined

This is peculiar, I tried running this test and was not able to reproduce it locally.

@kpollich
Copy link
Member Author

kpollich commented Jun 2, 2021

This is peculiar, I tried running this test and was not able to reproduce it locally.

My mistake - this was resolved late in the day yesterday over Slack in a310e4a and I didn't update the initial comment.

@nchaulet nchaulet self-requested a review June 2, 2021 14:27
@kpollich
Copy link
Member Author

kpollich commented Jun 2, 2021

Great work so far @kpollich !

I left a few comments in the code, not any blockers for now.

I tested locally and saw the following behaviour when view an integration package:

Screenshot 2021-06-02 at 12 09 44

It looks like this is one of the markdown renderers (https://github.com/kpollich/kibana/blob/b174e639dab53190c1393f1f7519cd9b259ac46d/x-pack/plugins/fleet/public/applications/integrations/sections/epm/screens/detail/overview/markdown_renderers.tsx#L62) receiving an unknown language $json. Not sure whether this is a known problem?

@jloleysens looks like this a known issue as of a few minutes ago. Something upstream. #101168

@paul-tavares paul-tavares self-requested a review June 2, 2021 15:13
@legrego legrego self-requested a review June 2, 2021 20:01
@kpollich
Copy link
Member Author

kpollich commented Jun 4, 2021

@jen-huang I've added screenshots + notes for each major UI change to the PR description as requested.

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

limits.yml LGTM

@kpollich
Copy link
Member Author

kpollich commented Jun 4, 2021

@elasticmachine merge upstream

@hbharding
Copy link
Contributor

Thanks @kpollich this is a great start and I appreciate the before/after screenshots. I haven't tested this locally, but I noticed a few layout + minor details in looking at the screenshots. I'll summarize here, and i've included annotated screenshots below for more detail. Also, just in case, here's the link to the Figma designs

  • Breadcrumbs on Integrations home page should match the tabs, "Browse" and "Manage". Currently they say "All" and "Installed"
  • The left/right margins in the content area (below header) should be white. Currently it appears to be using EuiPageBackgroundColor.
  • Remove the double/nested header on the Integration detail page (Nginx). The header with the graphic should only appear on the Integrations home page.
  • Remove Fleet's top bar navigation on the "Add Integration" page.
  • On the "Add Integration" page, the root breadcrumb should say Integrations rather than Fleet since this is the place users started from.
Annotated screenshots

image
image
image
image

@kpollich
Copy link
Member Author

kpollich commented Jun 4, 2021

Thanks @kpollich this is a great start and I appreciate the before/after screenshots. I haven't tested this locally, but I noticed a few layout + minor details in looking at the screenshots. I'll summarize here, and i've included annotated screenshots below for more detail. Also, just in case, here's the link to the Figma designs

  • Breadcrumbs on Integrations home page should match the tabs, "Browse" and "Manage". Currently they say "All" and "Installed"
  • The left/right margins in the content area (below header) should be white. Currently it appears to be using EuiPageBackgroundColor.
  • Remove the double/nested header on the Integration detail page (Nginx). The header with the graphic should only appear on the Integrations home page.
  • Remove Fleet's top bar navigation on the "Add Integration" page.
  • On the "Add Integration" page, the root breadcrumb should say Integrations rather than Fleet since this is the place users started from.

Annotated screenshots

Thanks so much for this, @hbharding - extremely helpful and actionable feedback. I've just pushed up fixes for everything here, and I'm including screenshots of the updates below. Thanks again!

Screenshots

review-add-integration

review-integration-details-nginx

review-integrations-home

@kpollich kpollich requested a review from legrego June 4, 2021 18:14
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
fleet 458 472 +14

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
fleet 980 981 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fleet 747.7KB 713.7KB -34.0KB
securitySolution 6.9MB 6.9MB +6.0B
total -33.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 347.5KB 425.1KB +77.6KB
Unknown metric groups

API count

id before after diff
fleet 1070 1071 +1

async chunk count

id before after diff
fleet 5 6 +1

History

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

cc @kpollich

@kpollich kpollich merged commit dc1d98b into elastic:master Jun 7, 2021
@kpollich kpollich deleted the integrations-separate-app branch June 7, 2021 14:44
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 99848

kpollich added a commit to kpollich/kibana that referenced this pull request Jun 7, 2021
* WIP: Re-create separation of integrations app

* Fix breadcrumbs

* Fix add integration button/routing

* Fix integrations test paths

* Fix failing policy tab tests

* Fix type errors

* Fix more type errors

* Fix integrations home page redirect

* Fix circular import

* Fix i18n errors

* Fix FTR paths

* Fix more deep fleet import paths

* Remove unneeded state set

* Fix more type errors

* Fix failing security_solutions tests

* Address redirect back path todo

* Fix page path in FTR

* Fix type error

* 🤞 Fix FTR failures

* Fix package details path in endpoint tests

* Fix test import

* Fix add integration route + breadcrumbs

* Fix missing layout for create package policy page

* Fixup Kibana feature declaration + fix app registry arrays

* Fix Kibana startup error from feature registration

* Fix telemetry schema

* Remove integrations from privilege tests

* Increase Fleet bundle limit by 20kb to fix CI error

* Use correct updated Fleet bundle limit

* Update limits.yml via script

* Address design feedback

* Fix i18n error

* Fix failing security solution tests

* Fix type error

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	packages/kbn-optimizer/limits.yml
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 7, 2021
* master: (90 commits)
  Fix UI breaks on providing long search keyword in 'Search Box' (elastic#101385)
  Adds css class to EuiDescriptionListDescription in order to break word on exception details card (elastic#101481)
  [Lens] Increase timings for drag and drop tests (elastic#101380)
  [Lens] Fix editor react error on configuration panel (elastic#101367)
  [Fleet] Move integrations to a separate app (elastic#99848)
  Fix incorrect message displayed on importing Timeline Templates (elastic#101288)
  [Cases] RBAC (elastic#95058)
  [APM] Visual improvements for new APM layout with left navigation (elastic#101360)
  [master] More precise alerts matching (elastic#99820)
  [Lens] Value in legend (elastic#101353)
  Revert "[Reporting] ILM policy for managing reporting indices (elastic#100130)" (elastic#101358)
  [Discover] Fix header row of data grid in Firefox (elastic#101374)
  Add link to advanced setting in Discover (elastic#101154)
  Url service locators (elastic#101045)
  [Timelion] Update the removal message to mention the exact version (elastic#100994)
  [Security Solution][Detection Engine] Test cases for alias failure test cases where we don't copy aliases correctly (elastic#101437)
  [Event Log] Adding `type_id` to saved object array in event log (elastic#100939)
  [Reporting] Add `location.url` info to console message logs (elastic#101427)
  [Security Solutions][Detection Engine] Fixes timestamp bugs within source indexes when the formats are not ISO8601 format (elastic#101349)
  Improve Task Manager instrumentation (elastic#99160)
  ...
kpollich added a commit that referenced this pull request Jun 7, 2021
* WIP: Re-create separation of integrations app

* Fix breadcrumbs

* Fix add integration button/routing

* Fix integrations test paths

* Fix failing policy tab tests

* Fix type errors

* Fix more type errors

* Fix integrations home page redirect

* Fix circular import

* Fix i18n errors

* Fix FTR paths

* Fix more deep fleet import paths

* Remove unneeded state set

* Fix more type errors

* Fix failing security_solutions tests

* Address redirect back path todo

* Fix page path in FTR

* Fix type error

* 🤞 Fix FTR failures

* Fix package details path in endpoint tests

* Fix test import

* Fix add integration route + breadcrumbs

* Fix missing layout for create package policy page

* Fixup Kibana feature declaration + fix app registry arrays

* Fix Kibana startup error from feature registration

* Fix telemetry schema

* Remove integrations from privilege tests

* Increase Fleet bundle limit by 20kb to fix CI error

* Use correct updated Fleet bundle limit

* Update limits.yml via script

* Address design feedback

* Fix i18n error

* Fix failing security solution tests

* Fix type error

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	packages/kbn-optimizer/limits.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged release_note:feature Makes this part of the condensed release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet