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

Add test for detecting new and removed SO types #104507

Merged
merged 7 commits into from
Jul 8, 2021

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Jul 6, 2021

Summary

Fixes #104418

This adds a test that checks that any first-party SO types that are removed are added to our query for documents that should be removed on the next upgrade. In the future, we may explore automating this by only migrating known document types or adding a dedicated API (#104246).

I ran this test against 7.8+ to find any types that have been removed since then and I found these 5 types and added them to the query so they will be filtered out on subsequent upgrades:

  • apm-services-telemetry
  • background-session
  • cases-sub-case
  • file-upload-telemetry
  • ml-telemetry

This needs to be backported to 7.14 so that any users with these documents can successfully upgrade.

Checklist

Delete any items that are not applicable to this PR.

@@ -415,11 +415,31 @@ describe('ElasticIndex', () => {
query: {
bool: {
must_not: [
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test probably needs to change so that we don't have to update every time a new type is added to REMOVED_TYPES

Copy link
Contributor

Choose a reason for hiding this comment

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

We could just import excludeUnusedTypesQuery for now to assert against, OTOH it would break once we register removed types at runtime, so...

@joshdover joshdover force-pushed the migrations/registration-tests branch from 86e6dc9 to a555b9a Compare July 6, 2021 17:09
import * as kbnTestServer from '../../../../test_helpers/kbn_server';

// Types should NEVER be removed from this array
const previouslyRegisteredTypes = [
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 ran this test on all branches from 7.7 forward. We can and should do it for the prior 7.x versions but it will take some extra work since the typeRegistry API didn't exist yet. Will do as a follow up.

/**
* Types that are no longer registered and need to be removed
*/
export const REMOVED_TYPES: string[] = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty basic for now, but can be improved in #104246

@@ -149,6 +149,7 @@ export interface SavedObjectsServiceSetup {
*/
export interface InternalSavedObjectsServiceSetup extends SavedObjectsServiceSetup {
status$: Observable<ServiceStatus<SavedObjectStatusMeta>>;
getTypeRegistry: () => ISavedObjectTypeRegistry;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly necessary, but makes the test faster since I don't have to wait for start to complete

Copy link
Contributor

Choose a reason for hiding this comment

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

The types are only guaranteed to be registered once all plugins completed their setup, but it's probably fine as long as we keep it internal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'll just add that to the docstring to make it clear

].sort();

describe('SO type registrations', () => {
it('does not remove types from registrations without updating unusedTypesQuery', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pgayvallet any thoughts on this approach before I continue?

@joshdover joshdover force-pushed the migrations/registration-tests branch from a555b9a to 92ec28b Compare July 7, 2021 12:18
@joshdover joshdover added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v7.15.0 v8.0.0 Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jul 7, 2021
@joshdover joshdover marked this pull request as ready for review July 7, 2021 15:26
@joshdover joshdover requested a review from a team as a code owner July 7, 2021 15:26
@elasticmachine
Copy link
Contributor

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

@joshdover joshdover added the auto-backport Deprecated - use backport:version if exact versions are needed label Jul 7, 2021
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.

Don't really like that we need to maintain an hardcoded list in the test file, but I don't see any other option, so I guess it will have to do.

@@ -415,11 +415,31 @@ describe('ElasticIndex', () => {
query: {
bool: {
must_not: [
{
Copy link
Contributor

Choose a reason for hiding this comment

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

We could just import excludeUnusedTypesQuery for now to assert against, OTOH it would break once we register removed types at runtime, so...

@@ -149,6 +149,7 @@ export interface SavedObjectsServiceSetup {
*/
export interface InternalSavedObjectsServiceSetup extends SavedObjectsServiceSetup {
status$: Observable<ServiceStatus<SavedObjectStatusMeta>>;
getTypeRegistry: () => ISavedObjectTypeRegistry;
Copy link
Contributor

Choose a reason for hiding this comment

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

The types are only guaranteed to be registered once all plugins completed their setup, but it's probably fine as long as we keep it internal

@joshdover joshdover enabled auto-merge (squash) July 7, 2021 16:07
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@joshdover joshdover merged commit 489a52b into elastic:master Jul 8, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 8, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.14
7.x

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jul 8, 2021
kibanamachine added a commit that referenced this pull request Jul 9, 2021
…4859)

* Add test for detecting new and removed SO types (#104507)

* Remove type not present on 7.14 branch

* Add additional old SO type from 7.6 to filter and test (#104913)

Co-authored-by: Josh Dover <[email protected]>
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 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 v7.14.0 v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test that ensures that SO types are not removed
4 participants