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] Remove migrations enableV2 config. #115655

Merged
merged 4 commits into from
Oct 20, 2021

Conversation

lukeelmers
Copy link
Member

@lukeelmers lukeelmers commented Oct 19, 2021

[skip-ci]

Part of #96396

This marks the enableV2 migrations config (deprecated in 7.12) as unused. As a result, users won't be able to use v1 migrations from 7.16.

Due to the way the SO config types were handled, it requires removing a few pieces of code, however it doesn't remove the full set of v1 migration code as outlined in #96396.

Comment on lines -176 to -177
// TODO migrationsV2: remove old migrations algorithm
if (this.soMigrationsConfig.enableV2) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to minimize the code that was removed here, but due to the way soMigrationsConfig is typed, I had to take this out.

x-pack/plugins/uptime/e2e/config.ts Show resolved Hide resolved
@lukeelmers lukeelmers force-pushed the feat/remove-v1-config branch from 86e7d0a to 15d039b Compare October 19, 2021 20:43
@kibanamachine

This comment has been minimized.

@lukeelmers
Copy link
Member Author

The one failure here is a unit test from the kibana_migrator suite: https://buildkite.com/elastic/kibana-pull-request/builds/521#87ef5d5b-7bf8-450a-a53a-72499774a19a

I think it just needs to be mocked differently for v2, but hadn't gotten to look more closely at it yet.

  | FAIL  src/core/server/saved_objects/migrations/kibana/kibana_migrator.test.ts
  | ● KibanaMigrator › runMigrations › only runs migrations once if called multiple times
  |  
  | expect(jest.fn()).toHaveBeenCalledTimes(expected)
  |  
  | Expected number of calls: 1
  | Received number of calls: 0
  |  
  | 146 \|       await migrator.runMigrations();
  | 147 \|
  | > 148 \|       expect(options.client.cat.templates).toHaveBeenCalledTimes(1);
  | \|                                            ^
  | 149 \|     });
  | 150 \|
  | 151 \|     it('emits results on getMigratorResult$()', async () => {
  |  
  | at Object.<anonymous> (src/core/server/saved_objects/migrations/kibana/kibana_migrator.test.ts:148:44)
  |  
  |  
  | Test Suites: 1 failed, 31 skipped, 6397 passed, 6398 of 6429 total
  | Tests:       1 failed, 218 skipped, 12 todo, 48231 passed, 48462 total
  | Snapshots:   7125 passed, 7125 total

@lukeelmers lukeelmers added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Oct 20, 2021
@lukeelmers lukeelmers marked this pull request as ready for review October 20, 2021 01:44
@lukeelmers lukeelmers requested review from a team as code owners October 20, 2021 01:44
@elasticmachine
Copy link
Contributor

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

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

kibana-docker LGTM

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Oct 20, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@jbudz
Copy link
Member

jbudz commented Oct 20, 2021

I just wanted to double check, is the plan to partially backport this to 7.16? Or are we removing the config altogether?

@lukeelmers
Copy link
Member Author

I just wanted to double check, is the plan to partially backport this to 7.16? Or are we removing the config altogether?

Core team is still discussing this. We had originally planned to just merge this for 8.0, but as we have explicitly documented that the config could be removed in a minor and it only existed for debugging purposes, we had a last minute discussion today about possibly backporting to 7.16 as well (to simplify long term support).

The config removal ended up being a bit more widespread than initially anticipated, so the rest of the team is going to decide tomorrow morning in EMEA whether we want to try to push this through before the first 7.16 BC is kicked off. If we can't make that happen, we'll stick with 8.0 only.

@rudolf rudolf added the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 20, 2021
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

Uptime changes LGTM !!

@rudolf rudolf enabled auto-merge (squash) October 20, 2021 09:34
@kibanamachine
Copy link
Contributor

kibanamachine commented Oct 20, 2021

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Jest Tests / Dashboard container lifecycle Old dashboard container is destroyed when new dashboardId is given

Metrics [docs]

✅ unchanged

History

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

cc @lukeelmers

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

APM changes LGTM

@lukeelmers
Copy link
Member Author

Chatted with @brianseeders -- the buildkite build is passing, but did not update the kibana-ci check on this PR after the test was retried. Ops team will investigate a fix for this, but in the meantime we should be safe to merge.

@lukeelmers lukeelmers disabled auto-merge October 20, 2021 15:17
@lukeelmers lukeelmers merged commit a7fff86 into elastic:master Oct 20, 2021
@lukeelmers lukeelmers deleted the feat/remove-v1-config branch October 20, 2021 15:17
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 115655

lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Oct 20, 2021
# Conflicts:
#	src/core/server/saved_objects/migrationsv2/integration_tests/migration_from_older_v1.test.ts
#	src/core/server/saved_objects/migrationsv2/integration_tests/migration_from_same_v1.test.ts
lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Oct 20, 2021
# Conflicts:
#	src/core/server/saved_objects/migrationsv2/integration_tests/migration_from_older_v1.test.ts
#	src/core/server/saved_objects/migrationsv2/integration_tests/migration_from_same_v1.test.ts
lukeelmers added a commit that referenced this pull request Oct 20, 2021
# Conflicts:
#	src/core/server/saved_objects/migrationsv2/integration_tests/migration_from_older_v1.test.ts
#	src/core/server/saved_objects/migrationsv2/integration_tests/migration_from_same_v1.test.ts
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 buildkite-ci Feature:Migrations 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:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants