Explicit namespaces for esFilters by lizozom · Pull Request #57177 · elastic/kibana

lizozom · 2020-02-10T09:20:50Z

Summary

This PR applies the proposed resolution for #52374, trying to balance code readability, API discoverability, DX and documentation generation using ApiExtractor on the filters helper namespace

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
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

…izozom/kibana into newplatform/data/kuery-query-namespace

…filters-namespace-O_O

…ata/filters-namespace-O_O

elasticmachine · 2020-02-11T12:45:53Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

lizozom · 2020-02-11T13:04:56Z

@elasticmachine merge upstream

flash1293

Kibana app changes LGTM

lizozom · 2020-02-11T16:16:58Z

...acy/plugins/siem/public/pages/detection_engine/rules/components/description_step/helpers.tsx

                <EuiBadgeWrap color="hollow">
                  {indexPatterns != null ? (
-                    <FilterLabel
+                    <esFilters.FilterLabel


I appreciate that!!!! That's great

nreese

maps changes LGTM

lukasolson

Left a few minor comments below. Overall, LGTM. Pulled down and tested in Chrome and things looked good.

lukasolson · 2020-02-11T20:55:41Z

src/legacy/core_plugins/data/public/filter/filter_manager/test_helpers/get_stub_filter.ts


 export function getFilter(
-  store: esFilters.FilterStateStore,
+  store: any, // I don't want to export only for this, as it should move to data plugin


Do you want to make a follow-up PR or issue for this so we don't forget?

Specifically, this piece of code should actually go away as soon as state management migration is done.
But I created an issue for other follow ups needed.

#57434

lukasolson · 2020-02-11T21:01:18Z

src/plugins/data/public/index.ts

+  createSavedQueryService,
+  syncAppFilters,
+  syncQuery,
+  getTime,
+  getQueryLog,
+  getQueryStateContainer,


Should these be put under a namespace of some sort?

Will get there! :)

lukasolson · 2020-02-11T21:05:27Z

src/plugins/data/server/index.ts

 import { PluginInitializerContext } from '../../../core/server';
 import { DataServerPlugin, DataPluginSetup, DataPluginStart } from './plugin';

+import {


Do we need to export a separate namespace for both public/server or can we create the namespace inside the common folder and just re-export here and in public?

We do need separate namespaces for public \ server as they have really different needs @lukeelmers

lukasolson · 2020-02-11T21:07:03Z

...lugins/siem/public/pages/detection_engine/rules/components/description_step/filter_label.tsx

@@ -1,95 +0,0 @@
-/*


I'm not sure I understand why these SIEM files were deleted... could you explain?

They duplicated files from our repo, requiring some really internal types that shouldn't be exported.
I instead exported FilterLabel and used that.

poffdeluxe

Canvas changes look good

…ata/filters-namespace-O_O

kibanamachine · 2020-02-12T11:08:49Z

💚 Build Succeeded

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

History

💚 Build #25854 succeeded e1685ec
💚 Build #25840 succeeded a916827
💚 Build #25642 succeeded e9b90d2
💚 Build #25502 succeeded 1ee974b
💔 Build #25487 failed 392ec66

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

lizozom · 2020-02-12T11:21:45Z

Forcing merge because I copied the fix from #57421 and now it requires an additional review from ML.

* Explicit namespaces for esQuery and esQuery * Remove unnecessary file from siem * remove jsonvalue definition from siem * esFilters refactor O_O * Delete IFilterType * import fixes * Fix merge Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* Explicit namespaces for esQuery and esQuery * Remove unnecessary file from siem * remove jsonvalue definition from siem * esFilters refactor O_O * Delete IFilterType * import fixes * Fix merge Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

FrankHassanabad

Looked at the server side and it is LGTM for us

Liza K and others added 7 commits February 9, 2020 18:29

Explicit namespaces for esQuery and esQuery

7c5636b

Remove unnecessary file from siem

bc01a5b

Merge branch 'master' into newplatform/data/kuery-query-namespace

fe08d7a

remove jsonvalue definition from siem

c1c5193

Merge branch 'newplatform/data/kuery-query-namespace' of github.com:l…

3dee295

…izozom/kibana into newplatform/data/kuery-query-namespace

esFilters refactor O_O

f9743b2

Delete IFilterType

392ec66

lizozom mentioned this pull request Feb 10, 2020

AppArch plugins documentation generation #56881

Closed

11 tasks

import fixes

1ee974b

lizozom mentioned this pull request Feb 10, 2020

moving visualize/utils to new platform #56650

Merged

Liza K added 4 commits February 10, 2020 18:01

Merge remote-tracking branch 'upstream/master' into newplatform/data/…

01be6d4

…filters-namespace-O_O

Merge remote-tracking branch 'upstream/master' into newplatform/data/…

e9b90d2

…filters-namespace-O_O

Merge branch 'master' of github.com:elastic/kibana into newplatform/d…

ea19f8f

…ata/filters-namespace-O_O

Fix merge

a916827

lizozom self-assigned this Feb 11, 2020

lizozom added Feature:New Platform Team:AppArch v7.7.0 v8.0.0 labels Feb 11, 2020

lizozom added the release_note:skip Skip the PR/issue when compiling release notes label Feb 11, 2020

Merge branch 'master' into newplatform/data/filters-namespace-O_O

e1685ec

lizozom marked this pull request as ready for review February 11, 2020 16:11

lizozom requested a review from a team February 11, 2020 16:11

lizozom requested review from a team as code owners February 11, 2020 16:11

flash1293 approved these changes Feb 11, 2020

View reviewed changes

lizozom commented Feb 11, 2020

View reviewed changes

nreese approved these changes Feb 11, 2020

View reviewed changes

lukasolson approved these changes Feb 11, 2020

View reviewed changes

poffdeluxe approved these changes Feb 11, 2020

View reviewed changes

Liza K added 2 commits February 12, 2020 10:50

Merge branch 'master' of github.com:elastic/kibana into newplatform/d…

03f5a90

…ata/filters-namespace-O_O

Merge fixes

d1a3d25

lizozom mentioned this pull request Feb 12, 2020

Explicit exports for data plugin #57434

Closed

8 tasks

lizozom merged commit b8c5b24 into elastic:master Feb 12, 2020

lizozom mentioned this pull request Feb 12, 2020

[7.x] Explicit namespaces for esFilters (#57177) #57440

Merged

FrankHassanabad reviewed Feb 12, 2020

View reviewed changes

Conversation

lizozom commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Feb 11, 2020

Uh oh!

lizozom commented Feb 11, 2020

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XavierM Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nreese left a comment

Choose a reason for hiding this comment

Uh oh!

lukasolson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizozom Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poffdeluxe left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Feb 12, 2020

💚 Build Succeeded

History

Uh oh!

lizozom commented Feb 12, 2020

Uh oh!

FrankHassanabad left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lizozom commented Feb 10, 2020 •

edited

Loading

XavierM Feb 12, 2020 •

edited

Loading

lizozom Feb 12, 2020 •

edited

Loading