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

Removes support for legacy exports #110738

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Sep 1, 2021

Summary

Removes support for legacy imports from the top most UI layer of saved objects management.
Handles Phase 1 of #103921

The documentation about importing saved objects only talks about importing .ndjson files, so we don't need to do anything there.

Checklist

Risk Matrix

Risk Probability Severity Mitigation/Notes
Saved objects in legacy format will not be usable Low Low Users should export their saved objects from the UI or rely on snapshots as backups for their data rather than raw exports from versions < 7 that are in the legacy format. We've been warning about the support for legacy format going away since 7.0, which should have given users plenty of time to convert their data.

For maintainers

Release note:

In Kibana 8.0, we will no longer support the legacy export format from Kibana 6.x. Importing saved objects using the UI is restricted to .ndjson format imports.

@TinaHeiligers TinaHeiligers added the release_note:skip Skip the PR/issue when compiling release notes label Sep 1, 2021
@TinaHeiligers TinaHeiligers added Feature:Saved Objects release_note:breaking v8.0.0 and removed release_note:skip Skip the PR/issue when compiling release notes labels Sep 1, 2021
@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@@ -16,11 +16,6 @@ jest.doMock('../../../lib/resolve_import_errors', () => ({
resolveImportErrors: resolveImportErrorsMock,
}));

export const importLegacyFileMock = jest.fn();
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 only removed the mock for the method we're removing here.

this.setState({ status: 'loading', error: undefined });

// Log warning on server, don't wait for response
logLegacyImport(http);
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 method and route will be removed in a follow up that handles the second bullet point of #103921

@@ -635,7 +440,8 @@ export class Flyout extends Component<FlyoutProps, FlyoutState> {
);
}

if (!isLegacyFile && status === 'success') {
// Import summary for completed import
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 sure if the code comment is needed here so I added one anyway. We only had a comment for handling legacy imports and that's gone now.

@TinaHeiligers TinaHeiligers marked this pull request as ready for review September 2, 2021 01:30
@TinaHeiligers TinaHeiligers requested review from a team as code owners September 2, 2021 01:31
@TinaHeiligers
Copy link
Contributor Author

TinaHeiligers commented Sep 2, 2021

@elastic/kibana-presentation I've had to address some failing functional tests that were either directly testing methods to handle legacy saved object imports or had legacy objects being imported for a test. Here's an issue for that: #111004

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.

A few comments, but looking fine.

Do you think the second bullet point of #103921 requires a distinct PR (the third definitely does though) ? It may be easier to review those two parts as the same time (but that's just an opinion, splitting is fine too)

Comment on lines 9 to 10
"panelsJSON": "[{\"gridData\":{\"h\":3,\"i\":\"1\",\"w\":6,\"x\":0,\"y\":0},\"id\":\"3fe22200-3dcb-11e8-8660-4d65aa086b3c\",\"panelIndex\":\"1\",\"type\":\"visualization\",\"version\":\"6.2.4\"}]",
"optionsJSON": "{\"hidePanelTitles\":false,\"useMargins\":true}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that fine to remove those files/tests , or should we just skip the tests and open an issue for the presentation team to adapt them?

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Sep 2, 2021

Choose a reason for hiding this comment

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

Um, good idea! Done in c0299e6 (#110738)
I've also created the follow up issue for the presentation team and let them know: #111004

Comment on lines -216 to -220
await esArchiver.load('test/functional/fixtures/es_archiver/saved_objects_imports');
await kibanaServer.uiSettings.replace({});
await PageObjects.settings.navigateTo();
await PageObjects.settings.clickKibanaSavedObjects();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove all the .json export files that should no longer be used in test/functional/apps/management/exports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Attempt 1: 912c588 (#110738)

Comment on lines 63 to 64
await this.testSubjects.existOrFail('importSavedObjectsSuccess', { timeout: 20000 });
await this.testSubjects.existOrFail('importSavedObjectsSuccess', { timeout: 20000 });
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this added by mistake?

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Sep 2, 2021

Choose a reason for hiding this comment

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

Oops, yes, I've removed the duplicate in 317a448 (#110738) 😊

Comment on lines -117 to -119
if (isLegacyFile) {
return overwriteRadio;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Now that this condition is removed, we no longer need to use this overwriteRadio variable, and we can directly inline it's value inside the returned value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TinaHeiligers
Copy link
Contributor Author

TinaHeiligers commented Sep 2, 2021

Do you think the second bullet point of #103921 requires a distinct PR

@pgayvallet I started looking into removing the files and it's a lot more work that one might think. I think it'll be easier to follow the split pattern and unblock other teams from moving forward with their work.

I've started to work on a draft and will follow up in new PR with the remaining parts as soon as priorities allow.

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers force-pushed the so-management/remove_legacy_import_support branch from ab16382 to 7fac104 Compare September 2, 2021 21:59
@pgayvallet
Copy link
Contributor

I started looking into removing the files and it's a lot more work that one might think.

Who would have expected that coming from an SO feature 😅 . Follow-up it is, then

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.

LGTM

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.

🎉

Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

Presentation changes lgtm

@TinaHeiligers TinaHeiligers merged commit 66cb058 into elastic:master Sep 3, 2021
@TinaHeiligers TinaHeiligers deleted the so-management/remove_legacy_import_support branch September 3, 2021 14:22
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
savedObjectsManagement 140.0KB 131.1KB -8.9KB

Page load bundle

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

id before after diff
savedObjectsManagement 38.3KB 37.5KB -818.0B

History

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

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 110738 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 7, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 110738 or prevent reminders by adding the backport:skip label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants