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] Prepare SO management for versionable types #149495

Merged
merged 39 commits into from
Feb 2, 2023

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Jan 25, 2023

Summary

Part of preparing HTTP APIs and associated interfaces for versioning:

Related #149098

Fixes #149495

@jloleysens jloleysens added 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 Feature:Saved Objects Management labels Jan 25, 2023
@jloleysens jloleysens marked this pull request as ready for review January 27, 2023 14:09
@jloleysens jloleysens requested a review from a team as a code owner January 27, 2023 14:09
@jloleysens
Copy link
Contributor Author

@TinaHeiligers @rudolf @pgayvallet I've tried to incorporate feedback. Let me know what you think of the current iteration:

  • using namespace for types (v1)
  • using latest.ts file as an optimisation on the export type CoolName = ... now that we removed V1 suffix from names.

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

The recent changes look good to me!

* Saved Object Management metadata associated with a saved object. See
* {@link SavedObjectWithMetadata}.
*/
export interface SavedObjectMetadata {
Copy link
Contributor

@TinaHeiligers TinaHeiligers Feb 2, 2023

Choose a reason for hiding this comment

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

Are the types that don't have the V1 suffix not expected to change at all? I'd expect to see all the types and interfaces declared in this file to only be the ones that change.

Shared types/interfaces that we can guarantee will never change could live elsewhere, for example, in the main types file.

Keeping the versioned-types file as small as possible helps us monitor changes over time.

}
);
objects: v1.BulkDeleteBodyHTTP
): Promise<v1.BulkDeleteResponseHTTP> {
Copy link
Contributor

@TinaHeiligers TinaHeiligers Feb 2, 2023

Choose a reason for hiding this comment

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

Should we indicate these are for internal purposes only?

/* @internal */
export function bulkDeleteObjects(...

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

It's so hard to get it right the first time, thanks for keeping up!
For now, I'm ok with the proposed changes.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
savedObjectsManagement 79 78 -1

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
savedObjectsManagement 117 140 +23

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
savedObjectsManagement 85.2KB 85.2KB +1.0B

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
savedObjectsManagement 0 2 +2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
savedObjectsManagement 19.4KB 19.3KB -67.0B
Unknown metric groups

API count

id before after diff
savedObjectsManagement 130 154 +24

References to deprecated APIs

id before after diff
savedObjectsManagement 57 43 -14

History

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

@jloleysens
Copy link
Contributor Author

@TinaHeiligers @rudolf @pgayvallet thanks for all the input so far! @pgayvallet and I spoke about the current approach:

For now, lets go with the general recommendation that you should version a collection/namespace of types, instead of individual types.

Ultimately, plugins can version domain types however they choose, but the assumption is that it will be common that a set of interfaces change together.

In the case of SO management plugin we have a few domains AFAICT:

  • saved object (with metadata)
  • relations
  • import/export (although this mostly lives in our core packages)

We could group these in individual domains that are versioned independently (where if one type changes, naturally others will change). So we could have domain: relationsV1.Relation and relationsV1.GetRelationHTTP, then relationsV2.Relation and relationsV2.GetRelationHTTP.

However, the current API surface area is quite small and I'm not sure this will yield value for that level of effort. So everything just lives under an unqualified v1 (or v2).

See the README.md for an explanation. Let me know what you think, but based on current comments I think we are aligned. Happy to make revisions in follow ups.

@jloleysens jloleysens merged commit df809bd into elastic:main Feb 2, 2023
@jloleysens jloleysens deleted the saved-objects-man-versioned-apis branch February 2, 2023 16:02
@kibanamachine kibanamachine added v8.7.0 backport:skip This commit does not require backporting labels Feb 2, 2023
@jloleysens
Copy link
Contributor Author

We can capture details not represented here when working on #149929

jloleysens added a commit that referenced this pull request Feb 16, 2023
## Summary

This PR addresses 4 TODOs identified in
#149495. We address these TODOs by
taking a closer look at actual use of flagged fields in public code:

1. `FindQueryHTTP['namespaceType']`: Probably OK to directly expose this
as it seems highly unlikely this value will change soon... We should
also be able to version it since it is a well known set.
2. `FindQueryHTTP['search']`: looks like it is allowing users to enter
_any_ search value against SO attribs. However, this functionality is
constrained because it only takes a search term and runs it against a
server-side determined search field. The risk is that consumers of the
server-side functionality do not keep their default search field
up-to-date.
3. `FindQueryHTTP['fields']`: Our UI only ever passes in `id` here. We
should consider a follow up PR to add some counter telemetry to track
usage of this option and consider removing it entirely.
4. `FindQueryHTTP['sortField']`: Lock this down to a well known set of
values (probably the riskiest change here).

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
jloleysens added a commit that referenced this pull request Mar 1, 2023
## Summary

Adds a dev docs guide for the core-approved versioning strategy. This
strategy is subject to some iteration but is based on the work we did
for the Saved Objects Management plugin in
#149495.

Closes #149929

## How to test

1. Run `./scripts/dev_docs.sh`
2. Navigate to "Versioning interfaces" in the side nav menu

---------

Co-authored-by: Luke Elmers <lukeelmers@gmail.com>
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
## Summary

Adds a dev docs guide for the core-approved versioning strategy. This
strategy is subject to some iteration but is based on the work we did
for the Saved Objects Management plugin in
elastic#149495.

Closes elastic#149929

## How to test

1. Run `./scripts/dev_docs.sh`
2. Navigate to "Versioning interfaces" in the side nav menu

---------

Co-authored-by: Luke Elmers <lukeelmers@gmail.com>
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 Feature:Saved Objects Management 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 v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants