[Endpoint] Integrate the Policy list with ingest datasources api #60548

paul-tavares · 2020-03-18T17:54:37Z

Summary

Remove the use of fake data generator
Uses the Ingest datasources REST API to retrieve a list of endpoint datasources (filters by datasource.package.name === 'endpoint')
Use Ingest datasources/{id} REST API to get details of a datasource
Adjust List view to show currently available data from API

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser

elasticmachine · 2020-03-18T17:54:40Z

Pinging @elastic/endpoint-management (Team:Endpoint Management)

elasticmachine · 2020-03-18T17:54:42Z

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

oatkiller · 2020-03-19T14:40:51Z

x-pack/plugins/endpoint/public/applications/endpoint/services/ingest.ts

+ * @param http
+ * @param options
+ */
+export const sendGetEndpoingDatasources = (


style suggestion: Endpoint is misspelled. Also, I'm not clear on the name of this function. Based on the doc comment, I would think endpointSpecificDatasource might be a good candidate for the name. Also, for clarity, would you consider adding an explicit return type?
I think Promise GetDatasourcesResponse> would be correct

Sounds good. I'll change the name to fetchEndpointSpecificDatasources().
FYI - These are in fact Endpoint datasources - we will define them in the endpoint package and when creating a new one, the package.name will be set to endpoint and tie back to the package from where it was defined.

oatkiller · 2020-03-19T14:43:12Z

x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list/middleware.ts

-          total,
-        },
-      });
+        dispatch({


would you consider moving the dispatch call outside the try block? If the reducer throws a runtime error, it'll be treated the same as if the HTTP call returned an error code.

ahh - yes. good point. I personally like to keep the code inside of try {} blocks to a minimum since (I think) the JS compilers are not able to optimize that code. will move it out.

oatkiller · 2020-03-19T14:49:38Z

x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list/middleware.ts

+            total,
+          },
+        });
+      } catch (err) {


Since err will be any, the dispatch call accepts it even though the runtime value may not satisfy the expected payload type.

I can't just pass the err through because the ApiServerError structure is in the body property of the error object returned by http.* methods, thus why I do the check below. I test this by adding a kuery param on the fetch above to make it fail 😞

oatkiller · 2020-03-19T14:51:02Z

x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list/reducer.ts

+  }
+
+  if (action.type === 'userShownPolicyListServerFailedMessage') {
+    // Make sure that the apiError currently stored is the one the user was shown


Trying to understand the flow here. If an error is stored in state, the react view uses the toast services to show info about it, then dispatches this. If the error matches the one in state, we clear it.

What is the point of keeping track of that in state?

I'm trying to protect against showing the same error multiple times - example: during a quick paginations. That being said, I just realized that useEffect() has the error set as the dependency, so it will not fire multiple times unless it changes. Will look at it a little closer and remove it is not needed (will change API fetching middleware/reducer to clear error on fetch)

oatkiller · 2020-03-19T14:53:41Z

x-pack/plugins/endpoint/public/applications/endpoint/view/policy/policy_list.tsx

+            // eslint-disable-next-line @elastic/eui/href-or-on-click
+            <EuiLink
+              href={`${services.application.getUrlForApp('ingestManager')}#/configs/${version}`}
+              onClick={(ev: SyntheticEvent) => {


What is the reason for having the onClick here?

It prevents a full browser refresh.

This might be avoidable - a few weeks ago one of the updates from the kibana-platform team was a change required by Plugins for how to use react-router. Mainly, I think they are now passing down to us an instance of their history which we should use in initializing our router (or just use react-router's sub-routes - maybe?). If we do use their history instance, then doing getting the history object from react-router (using the hook) and then doing a history.push might do the same thing as this code below.

I don't recall if we have an issue open to look into that routing change.

BTW - @oatkiller question: I pinged the EUI team on why the @elastic/eui/href-or-on-click was enable, especially since we are mostly all using a router with history push. They said it was ok to turn it off. My question: should we turn it off across our plugin? (by introducing a .eslingrc.js file)

Gotcha. Looking through the code I see a few places that are using a similar pattern.

I wrote this up a while ago, but would like your thoughts: oatkiller@d6861d8

Seeing that you're doing navigateToApp, instead of history.push, I don't think the commit would work for you out of the box. But I would like to have an easy way to reuse the logic that guards event.preventDefault

also @parkiino cause I was talking to her as well.

re : changing lint rules. I've been contemplating changing lint rules since day 0. @stacey-gammon argued that having consistent rules across all of Kibana is worthwhile because it allows developers to contribute across the app with the same standards (and allows code to be theoretically copy-pasted around the app as well.)

In the past I used lint rules (and other forms of static analysis) heavily as a way to enforce consistency across the app, so it's something I still think about.

With that in mind, I think we should try to argue for the lint rules changes to happen Kibana-wide. If Kibana contributors at large disagree'd with our change, we could reevaluate then.

I think we should try to argue for the lint rules changes to happen Kibana-wide. If Kibana contributors at large disagree'd with our change, we could reevaluate then.

💯

For this particular issue, I think this should be a moot point once everything is converted to NP, there will no longer be the full page refresh navigating among apps. Though I'm surprised using navigateToApp somehow gets around the full page refresh. So maybe I am missing something else. cc @joshdover

Maybe we're hitting the issue because we have not implemented the recent change in the NP to use the history instance provided by plugin mount() (this change #56705 )???
Will try that out to see if that is it.

Ok, I tried it by using history.push() instead of navigateToApp() after making changes in our router to use the history instance passed by mount(), but still got full page refresh. Perhaps I'm missing something as to how we should interop with the core top-level router ( @joshdover ?? 😃 )

I'm laving in the code that uses navigateToApp() for now and will refactor if there is a better pattern to follow.

From platform team:

Yea, we have an open issue to discuss about the best way to avoid that, and to allow 'plain' links to have the same behavior as navigateToApp . The onClick workaround used in the PR is the way to go for now unfortunately

Sorry for the extra work!

@stacey-gammon no problem at all - appreciate the feedback.
Good to know that the platform team has this on their list of items to address in the future. The ability to "just handle it" is exactly what @chandlerprall suggested on Slack when I first approach the EUI team about possibly turning this ESLint rule off -

"...I recommend having an onClick at the document.body level that can programmatically interact with the router...."

…-policy-list-api-integration

oatkiller · 2020-03-19T20:31:12Z

x-pack/test/functional/apps/endpoint/policy_list.ts

@@ -25,14 +24,16 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {
      const policyTitle = await testSubjects.getVisibleText('policyViewTitle');
      expect(policyTitle).to.equal('Policies');
    });
-    it('shows policy count total', async () => {
+    // FIXME: Skipped until we can figure out how to load data for Ingest
+    it.skip('shows policy count total', async () => {


perhaps just remove these tests? they'll be available in git history

I'm thinking these will come right back in the not too far future.

…-policy-list-api-integration

paul-tavares · 2020-03-20T14:11:41Z

@elasticmachine merge upstream

…-policy-list-api-integration

kibanamachine · 2020-03-20T20:21:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ae0c332

History

💔 Build #34907 failed 42f293c
💔 Build #34881 failed 76b2cf7

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

kibanamachine · 2020-03-21T20:38:47Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 60548 or prevent reminders by adding the backport:skip label.

kibanamachine · 2020-03-22T20:48:45Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 60548 or prevent reminders by adding the backport:skip label.

kibanamachine · 2020-03-23T20:58:46Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

) (#60904) * Use ingest API to get endpoint datasources * Add `application` service to `KibanaContextProvider` * Adjust Policy list to show data available from API * Added ingest service + refactored middleware * handle api failures/errors * Removed policy list fake_data generator * Fix typing * Rename method + added explicit return type * move dispatch outside of try block * Removed unnecessary action * Added FIXME comments with link to issue * Skip some functional tests * added tests for ingest service * Policy list tests - turn it all off Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/endpoint/public/applications/endpoint/index.tsx # x-pack/plugins/endpoint/public/applications/endpoint/view/policy/policy_list.tsx

paul-tavares added 3 commits March 18, 2020 12:26

Use ingest API to get endpoint datasources

42e686e

Add application service to KibanaContextProvider

cb0d357

Adjust Policy list to show data available from API

fccbe1d

paul-tavares added WIP Work in progress v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Team:Endpoint Management Feature:Endpoint Elastic Endpoint feature labels Mar 18, 2020

paul-tavares self-assigned this Mar 18, 2020

paul-tavares added 4 commits March 18, 2020 15:07

Added ingest service + refactored middleware

5fab1ab

handle api failures/errors

d3c8aa3

Removed policy list fake_data generator

4d9fab8

Fix typing

75c70ac

paul-tavares marked this pull request as ready for review March 18, 2020 20:46

paul-tavares requested a review from a team as a code owner March 18, 2020 20:46

oatkiller reviewed Mar 19, 2020

View reviewed changes

paul-tavares added 7 commits March 19, 2020 12:48

Rename method + added explicit return type

68b0271

move dispatch outside of try block

006f33b

Removed unnecessary action

b121976

Merge remote-tracking branch 'upstream/master' into task/endpoint-150…

79a1b51

…-policy-list-api-integration

Added FIXME comments with link to issue

484c99c

Skip some functional tests

3ee7294

added tests for ingest service

9891023

oatkiller reviewed Mar 19, 2020

View reviewed changes

paul-tavares added 2 commits March 20, 2020 09:21

Policy list tests - turn it all off

d6b39be

Merge remote-tracking branch 'upstream/master' into task/endpoint-150…

76b2cf7

…-policy-list-api-integration

Merge branch 'master' into task/endpoint-150-policy-list-api-integration

42f293c

parkiino approved these changes Mar 20, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into task/endpoint-150…

ae0c332

…-policy-list-api-integration

paul-tavares merged commit 7983d1d into elastic:master Mar 20, 2020

paul-tavares deleted the task/endpoint-150-policy-list-api-integration branch March 20, 2020 20:33

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 21, 2020

paul-tavares mentioned this pull request Mar 23, 2020

[7.x] [Endpoint] Integrate the Policy list with ingest datasources api (#60548) #60904

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Endpoint] Integrate the Policy list with ingest datasources api #60548

[Endpoint] Integrate the Policy list with ingest datasources api #60548

paul-tavares commented Mar 18, 2020 •

edited

Loading

elasticmachine commented Mar 18, 2020

elasticmachine commented Mar 18, 2020

oatkiller Mar 19, 2020

paul-tavares Mar 19, 2020

oatkiller Mar 19, 2020

paul-tavares Mar 19, 2020

oatkiller Mar 19, 2020

paul-tavares Mar 19, 2020

oatkiller Mar 19, 2020

paul-tavares Mar 19, 2020

oatkiller Mar 19, 2020

paul-tavares Mar 19, 2020

paul-tavares Mar 19, 2020

oatkiller Mar 19, 2020

oatkiller Mar 19, 2020

stacey-gammon Mar 20, 2020

paul-tavares Mar 20, 2020

paul-tavares Mar 20, 2020

stacey-gammon Mar 23, 2020

paul-tavares Mar 23, 2020

oatkiller Mar 19, 2020

paul-tavares Mar 19, 2020

paul-tavares commented Mar 20, 2020

kibanamachine commented Mar 20, 2020

kibanamachine commented Mar 21, 2020

kibanamachine commented Mar 22, 2020

kibanamachine commented Mar 23, 2020

[Endpoint] Integrate the Policy list with ingest datasources api #60548

[Endpoint] Integrate the Policy list with ingest datasources api #60548

Conversation

paul-tavares commented Mar 18, 2020 • edited Loading

Summary

Checklist

elasticmachine commented Mar 18, 2020

elasticmachine commented Mar 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-tavares commented Mar 20, 2020

kibanamachine commented Mar 20, 2020

💚 Build Succeeded

History

kibanamachine commented Mar 21, 2020

kibanamachine commented Mar 22, 2020

kibanamachine commented Mar 23, 2020

paul-tavares commented Mar 18, 2020 •

edited

Loading