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 extensions refactor merge #142878

Merged
merged 67 commits into from
Nov 18, 2022

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Oct 6, 2022

Merges the changes of #134395 into the new packages structure.
Resolves #133835

Description

This PR represents a fully manual merge of the saved objects refactor of client wrapper system into repository extensions. These changes are being manually merged due to significant changes of the saved objects implementation in the main branch, specifically the migration to the new packages structure.

Other changes

Adds public types for extensions, temporarily omiting these from public mocks and saved objects service

Adds extensions to saved objects start contract, temporarily omitting them from the saved objects service start implementation

adding extensions to saved objects implementation

Still wip adding extensions and factories to implementation

more small changes

Starts removing wrappers

Updates saved objects service mock

Updates more types around extensions and wrappers removal

Adds saved objects extensions to plugin_context

Adds extensions mocks to public saved objects server mocks

Updates two more tests
@jeramysoucy jeramysoucy changed the title [WIP] POC for refactoring saved objects repository [WIP] Saved objects extensions refactor merge Oct 10, 2022
@jeramysoucy jeramysoucy self-assigned this Oct 12, 2022
@jeramysoucy
Copy link
Contributor

buildkite test this

@jeramysoucy jeramysoucy requested a review from a team as a code owner November 15, 2022 07:56
@cla-checker-service
Copy link

cla-checker-service bot commented Nov 15, 2022

💚 CLA has been signed

Copy link
Contributor

@dasansol92 dasansol92 left a comment

Choose a reason for hiding this comment

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

OLM changes LGTM (code review only)

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

Even if I think we need to discuss about the follow-ups and their prioritization, ihmo the PR is safe to be merged in its current state, so ihmo we should do it asap to free the owner from the burden of maintaining/rebasing the PR.

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

As discussed on Slack, I think there's room for better decoupling the repository from the extensions which could help simplify the complexity that this PR adds to the repository. But on the whole this PR improves the overall complexity so we don't need to block on that but can iterate on it in follow-up PRs.

Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

image

@azasypkin azasypkin self-requested a review November 17, 2022 07:15
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM, great job! Let's file issues for the most contentious changes and deal with them in the follow-ups.

Comment on lines +362 to +366
// If a user tries to create an object with `initialNamespaces: ['*']`, they need to have 'create' privileges for the Global Resource
// (e.g., All privileges for All Spaces).
// Inversely, if a user tries to overwrite an object that already exists in '*', they don't need to 'create' privileges for the Global
// Resource, so in that case we have to filter out that string from spacesToAuthorize (because `allowGlobalResource: true` is used
// below.)
Copy link
Member

Choose a reason for hiding this comment

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

TL;DR - We could move some of the logic into the extension, but I am inclined to leave as is for now and open an issue to investigate the impact and determine a clear delineation of responsibilities between the repo and extension.

++ It's worth exploring in the follow-up for sure. We had a somewhat related discussion in the original PR as well and figured that it wasn't an easy change, we might have a better outcome if we deal with this in isolation from other changes.

…saved_objects_client.ts

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>
@jeramysoucy
Copy link
Contributor

@azasypkin I think this is considerably better than what was there. Templating, or I guess "generics" in typescript, was the right answer. Thanks!

@jeramysoucy jeramysoucy added v8.7.0 and removed v8.6.0 labels Nov 17, 2022
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #1 / Alerts timeline Privileges: read only "before each" hook for "should not allow user with read only privileges to attach alerts to a new case"
  • [job] [logs] Rules, Alerts and Exceptions ResponseOps Cypress Tests on Security Solution / Detection rules, bulk edit, data view Overwrite index patterns in custom rules with configured data view

Metrics [docs]

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
@kbn/core-saved-objects-api-browser 76 0 -76
@kbn/core-saved-objects-api-server 137 0 -137
@kbn/core-saved-objects-api-server-internal 51 54 +3
@kbn/core-saved-objects-api-server-mocks 6 11 +5
@kbn/core-saved-objects-common 41 39 -2
@kbn/core-saved-objects-migration-server-internal 75 78 +3
@kbn/core-saved-objects-server 83 90 +7
@kbn/core-saved-objects-utils-server 86 84 -2
core 1202 1007 -195
data 2555 2553 -2
dataViews 228 227 -1
total -397

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/core-saved-objects-api-server 0 1 +1
@kbn/core-saved-objects-migration-server-internal 43 44 +1
total +2
Unknown metric groups

API count

id before after diff
@kbn/core-saved-objects-api-server 310 313 +3
@kbn/core-saved-objects-api-server-internal 71 75 +4
@kbn/core-saved-objects-api-server-mocks 6 11 +5
@kbn/core-saved-objects-migration-server-internal 101 110 +9
@kbn/core-saved-objects-server 226 297 +71
core 2708 2796 +88
total +180

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
@kbn/core-saved-objects-api-server-internal 5 6 +1
enterpriseSearch 19 21 +2
fleet 59 65 +6
osquery 108 113 +5
securitySolution 441 447 +6
total +20

Total ESLint disabled count

id before after diff
@kbn/core-saved-objects-api-server-internal 6 7 +1
enterpriseSearch 20 22 +2
fleet 67 73 +6
osquery 109 115 +6
securitySolution 518 524 +6
total +21

History

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

cc @jeramysoucy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting 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:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Saved Object Client wrappers