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

Move Elasticsearch type definitions out of APM #83081

Merged
merged 3 commits into from
Nov 11, 2020

Conversation

smith
Copy link
Contributor

@smith smith commented Nov 10, 2020

...and into x-pack.

Also remove PromiseReturnType from APM and use the copy in observability everywhere.

All of the additional changes to APM imports are just automatic sorting.

This makes doing #77720 a little easier and removes some implicit circular dependencies for #80508.

...and into x-pack.

Also remove `PromiseReturnType` from APM and use the copy in observability everywhere.

All of the additional changes to APM imports are just automatic sorting.

This makes doing elastic#77720 a little easier and removes some implicit circular dependencies for elastic#80508.
@smith smith added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 10, 2020
@smith smith requested review from a team as code owners November 10, 2020 17:40
@smith smith requested a review from a team November 10, 2020 17:40
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Nov 10, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Lens changes look good 🆗

@@ -7,21 +7,21 @@
import { schema } from '@kbn/config-schema';
import { Observable } from 'rxjs';
import { take } from 'rxjs/operators';
import { getDurationFormatter } from '../../../common/utils/formatters';
import { ProcessorEvent } from '../../../common/processor_event';
Copy link
Member

@sorenlouv sorenlouv Nov 10, 2020

Choose a reason for hiding this comment

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

Didn't we previously have an eslint rule that'd strictly order the imports alphabetically? Then we'd avoid this kind of diff.

I use that for backport: https://github.com/sqren/backport/blob/9c41c5bcd559f1b1b7a782a940143d28d1027c79/.eslintrc.js#L25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, but once they are ordered properly this kind of diff doesn't need to happen again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ordering I'm doing here is with VSCode's options-shift-o, which invokes TypeScript's organize imports feature.

We have some ordering rules in ESLint, but it doesn't fully check for TypeScript's suggested ordering. There's an open issue (typescript-eslint/typescript-eslint#113) to get it implemented in the ESLint TypeScript plugin that's not yet completed.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, it's a little harder to review this PR because of the import changes. I would prefer to have something that is enforced & automatic, or to not do this at all. I do agree that there is value in sorting the imports.

Copy link
Member

@sorenlouv sorenlouv Nov 11, 2020

Choose a reason for hiding this comment

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

Agree, there is value in sorting imports and it should be enforced and automatic. For backport unsorted imports make the eslint check fail:

image

image

I would be surprised if we can't have the same behaviour in Kibana.

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Task manager changes LGTM

Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @smith. I pushed a commit to fix some imports that were not updated.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 42768 42766 -2

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id before after diff
fleet-agents 25 23 -2
ingest-outputs 11 10 -1
ingest-package-policies 35 30 -5
lens 7 6 -1
total -9

History

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

@smith smith merged commit bc2da67 into elastic:master Nov 11, 2020
@smith smith deleted the nls/es-typings branch November 11, 2020 22:23
smith added a commit to smith/kibana that referenced this pull request Nov 11, 2020
...and into x-pack.

Also remove `PromiseReturnType` from APM and use the copy in observability everywhere.

All of the additional changes to APM imports are just automatic sorting.

This makes doing elastic#77720 a little easier and removes some implicit circular dependencies for elastic#80508.

Co-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co>
# Conflicts:
#	x-pack/plugins/apm/public/utils/testHelpers.tsx
smith added a commit that referenced this pull request Nov 12, 2020
...and into x-pack.

Also remove `PromiseReturnType` from APM and use the copy in observability everywhere.

All of the additional changes to APM imports are just automatic sorting.

This makes doing #77720 a little easier and removes some implicit circular dependencies for #80508.

Co-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co>
# Conflicts:
#	x-pack/plugins/apm/public/utils/testHelpers.tsx
pgayvallet pushed a commit to pgayvallet/kibana that referenced this pull request Nov 12, 2020
...and into x-pack.

Also remove `PromiseReturnType` from APM and use the copy in observability everywhere.

All of the additional changes to APM imports are just automatic sorting.

This makes doing elastic#77720 a little easier and removes some implicit circular dependencies for elastic#80508.

Co-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 12, 2020
…ts-public

* upstream/master: (57 commits)
  Remove unused asciidoc file (elastic#83228)
  [Lens] Remove background from lens embeddable (elastic#83061)
  [Discover] Unskip flaky tests based on discover fixture index pattern (elastic#82991)
  Removing unnecessary trailing slash in CODEOWNERS
  Trying to fix CODEOWNERS again, where was a non-existent team prior (elastic#83236)
  Trying to fix CODEOWERS, missing a starting slash (elastic#83233)
  skip flaky suite (elastic#83231)
  Add enzyme rerender test helper (elastic#83208)
  Move Elasticsearch type definitions out of APM (elastic#83081)
  [ts/checkTsProjects] produce a more useful error message (elastic#83209)
  [kbnClient] retry updating config if necessary (elastic#83205)
  I accidentally removed this line in a recent PR (elastic#83201)
  Don't make the caller do work the function can do (elastic#83180)
  [App Search] Update EngineRouter & EngineNav to use EngineLogic (elastic#83138)
  [Workplace Search] Add routes for Sources (elastic#83125)
  Update logstash pipeline management to use system index APIs (elastic#80405)
  [ML] Replace EuiBasicTable with EuiInMemoryTable (elastic#83057)
  [Metrics UI] Add basic interaction and shell for node details overlay (elastic#82013)
  [App Search] Added the log retention confirmation modal to the Settings page (elastic#83009)
  [docs] Fix create map title in import geospatial page (elastic#83172)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 12, 2020
… alerts/action-groups-as-conditions

* origin/alerts/stack-alerts-public: (91 commits)
  removed import from plugin code as it causes FTR to fail
  [Advanced Settings] Introducing telemetry (elastic#82860)
  [alerts] add executionStatus to event log doc for action execute (elastic#82401)
  Add additional sources routes (elastic#83227)
  [ML] Persisted URL state for the "Anomaly detection jobs" page (elastic#83149)
  [Logs UI] Add pagination to the log stream shared component (elastic#81193)
  [Index Management] Add an index template link to data stream details (elastic#82592)
  Add maps_oss folder to code_owners (elastic#83204)
  fix truncation issue (elastic#83000)
  [Ingest Manger] Move asset getters out of registry (elastic#83214)
  make defaulted field non maybe
  Remove unused asciidoc file (elastic#83228)
  [Lens] Remove background from lens embeddable (elastic#83061)
  [Discover] Unskip flaky tests based on discover fixture index pattern (elastic#82991)
  Removing unnecessary trailing slash in CODEOWNERS
  Trying to fix CODEOWNERS again, where was a non-existent team prior (elastic#83236)
  Trying to fix CODEOWERS, missing a starting slash (elastic#83233)
  skip flaky suite (elastic#83231)
  Add enzyme rerender test helper (elastic#83208)
  Move Elasticsearch type definitions out of APM (elastic#83081)
  ...
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:APM All issues that need APM UI Team support v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants