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

[Saved Objects] Import SO types from server rather than common to avoid deprecation #149289

Merged
merged 8 commits into from
Jan 25, 2023

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Jan 20, 2023

Summary

After merging #148979 there are a number of imports that can be fixed immediately to address our new deprecation notice.

To Core reviewers

The package core-saved-objects-server is using types from core-saved-objects-api-server which creates a circular dependency when using SavedObject type from it's new home in core-saved-object-server:

core-saved-objects-server -> core-saved-objects-api-server -> core-saved-objects-server

One solution is that we can create a new package packages/core/saved-objects/core-saved-objects-server-shared that will only hold the SavedObject type and a select few others. I'm not sure what the best approach here is. I have left core-saved-objects-api-server unchanged for now (i.e., it is still importing SavedObject from common which is deprecated).

Any input would be greatly appreciated!

@jloleysens jloleysens added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Saved Objects release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v8.7.0 labels Jan 20, 2023
@jloleysens jloleysens force-pushed the import-so-types-from-server branch from 725af0e to cd0806d Compare January 23, 2023 10:21
@jloleysens jloleysens marked this pull request as ready for review January 23, 2023 13:15
@jloleysens jloleysens requested review from a team as code owners January 23, 2023 13:15
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@botelastic botelastic bot added Team:Fleet Team label for Observability Data Collection Fleet team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jan 23, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

Fleet changes 🚀

@@ -7,7 +7,7 @@
*/

import { isNotFoundFromUnsupportedServer } from '@kbn/core-elasticsearch-server-internal';
import type { SavedObject } from '@kbn/core-saved-objects-common';
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah... It kinda awkward having to do this to avoid the deprecation, given both types are supposed to be the same, but if that's the direction we're willing to take, I guess it's fine.

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

ResponseOps changes LGTM!

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Visualizations team changes LGTM

@jeramysoucy jeramysoucy self-requested a review January 23, 2023 16:41
Copy link
Contributor

@jeramysoucy jeramysoucy left a comment

Choose a reason for hiding this comment

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

These changes look fine. Is the plan to remove the declarations in common entirely in the next major version release?

* main: (54 commits)
  [APM] Allow calling `createInternalESClient` without `context` (elastic#149320)
  [Synthetics] Errors list active state (elastic#149387)
  [FTRs] Execution Context: fix no data flakiness (elastic#149406)
  [Cloud Posture] - Deprecate csp rule remove migration object (elastic#148530)
  Bump elasticsearch-js to 8.6.0-canary.3 (elastic#148521)
  [Fleet] Use optimistic locking when updating `installed_es` on input package policy creation (elastic#148883)
  [ML] Remove job_type from job definitions in modules (elastic#149247)
  [ML] Consolidate `query_utils` into package `@kbn/ml-query-utils` (elastic#149224)
  [Synthetics] Better formatting for waterfall timeline tooltips (elastic#149142)
  [Cloud Posture] CIS AWS support - changes to findings tables (elastic#148945)
  [Lens] Enable previous time shift when using a date histogram (elastic#149126)
  [Synthetics] Object types panel and thresholds (elastic#149099)
  [Fleet] added back batch exec for update tags (elastic#148618)
  148790 - Fix scroll style for setup guide flyout (elastic#149242)
  Fix a11y issue with dev tool tabs (elastic#149349)
  [APM] Fix mobile indices (elastic#149230)
  [Dashboard] Fix Phrase_filter query for scripted fields (elastic#148942) (elastic#148943)
  renderCustomActionsRow with named params instead of args (elastic#149304)
  [ML] Adding ML execution context to es requests (elastic#148746)
  [Custom Branding] Replace EuiLoadingElastic with EuiLoadingSpinner (elastic#149261)
  ...
@jloleysens jloleysens enabled auto-merge (squash) January 24, 2023 15:12
@jeramysoucy
Copy link
Contributor

@jloleysens FYI I ran into some circular references between the core-saved-objects-api-server and core-saved-objects-server packages after updating the imports of the SavedObject type. Fortunately, this was the only issue I ran into in all of the core-saved-objects-* packages regarding this change. I'll be merging my updates in this PR, which should be wrapped up soon.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id before after diff
@kbn/core-saved-objects-api-server-internal 239 0 -239
@kbn/core-saved-objects-import-export-server-internal 692 70 -622
@kbn/core-saved-objects-server 94 1 -93
@kbn/core-saved-objects-server-internal 69 4 -65
alerting 134 71 -63
cases 194 144 -50
encryptedSavedObjects 16 6 -10
fleet 99 79 -20
lens 220 210 -10
observability 80 70 -10
security 75 65 -10
synthetics 174 164 -10
total -1202

History

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

cc @jloleysens

@jloleysens jloleysens merged commit 631675b into elastic:main Jan 25, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 25, 2023
@jloleysens jloleysens deleted the import-so-types-from-server branch January 26, 2023 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed backport:skip This commit does not require backporting chore Feature:Saved Objects release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Fleet Team label for Observability Data Collection Fleet team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants