[ZDT] set `switchToModelVersionAt` to `8.10.0` for all types by pgayvallet · Pull Request #158267 · elastic/kibana

pgayvallet · 2023-05-23T13:19:51Z

Summary

Sets switchToModelVersionAt to 8.10.0 for all registered SO types, forcing them to switch to the model version API once main is on version 8.10.0 (~end of june)

pgayvallet

Self review

pgayvallet · 2023-05-23T15:03:29Z

packages/core/saved-objects/core-saved-objects-server-internal/src/saved_objects_service.ts

          throw new Error('cannot call `registerType` after service startup.');
        }
-        this.typeRegistry.registerType(type);
+        this.typeRegistry.registerType(applyTypeDefaults(type));


Doing that directly in the type registry class was too low level, just breaking too many tests. Also I think it makes sense to have this done from the service instead.

pgayvallet · 2023-05-23T15:03:40Z

packages/core/saved-objects/core-saved-objects-server-internal/src/apply_type_defaults.ts

+  let switchToModelVersionAt = type.switchToModelVersionAt;
+  if (
+    !switchToModelVersionAt ||
+    !Semver.valid(switchToModelVersionAt) ||
+    Semver.gt(switchToModelVersionAt, globalSwitchToModelVersionAt)
+  ) {
+    switchToModelVersionAt = globalSwitchToModelVersionAt;
+  }
+
+  return {
+    ...type,
+    switchToModelVersionAt,
+  };


The actual logic

pgayvallet · 2023-05-23T15:04:09Z

...core/server/integration_tests/saved_objects/migrations/group2/check_registered_types.test.ts

We're adding switchToModelVersionAt on all types, so the associated hashes are changing, had to update the snapshot

elasticmachine · 2023-05-23T15:08:35Z

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

jloleysens · 2023-05-23T15:18:18Z

packages/core/saved-objects/core-saved-objects-server-internal/src/apply_type_defaults.ts

+    !Semver.valid(switchToModelVersionAt) ||
+    Semver.gt(switchToModelVersionAt, globalSwitchToModelVersionAt)


I would normally think of these lines as reasons to throw an error, something like:

Invalid version detected: "foobar". Please use SemVer compliant versions. for (1)

Max switch version is 8.10.0, you provided 8.11.0. for (2).

In which case we have more of a classic default behaviour. But I get that we may not want to draw attention/discussion quite yet.

After thinking about it, I think you're right. It's probably better to throw if the provided version is invalid (bad format) or higher than 8.10.0 (or 8.11.0 if we want to already allow owners to specify a one-version grace period)

jloleysens

Overall looks good, left a comment but I'll defer to @rudolf for final approval!

afharo · 2023-05-23T15:21:03Z

packages/core/saved-objects/core-saved-objects-server-internal/src/apply_type_defaults.test.ts

+    it(`sets switchToModelVersionAt to the global version if type version is greater than the global version`, () => {
+      const type = createType({
+        switchToModelVersionAt: '9.2.0',
+      });
+
+      const result = applyTypeDefaults(type);
+      expect(result.switchToModelVersionAt).toEqual(globalSwitchToModelVersionAt);
+    });


should this be allowed as we mentioned we needed a workaround to give teams some extra time?

We could use a delaySwitchToModelVersionAt and override the default or not apply the forced switch to those. That way, we need to agree on which types we're more lenient with based on the type-owner's justification but that can be done as a follow up.
Given the timing, we do need to force our hand a bit here.

Let's start by not :) and figure out if we need it then

IMO, there is no need for an additional property. The way I understood our conversation was:

If not provided, let's set 8.10.0 as the default switchover deadline.
Teams can override that default by either anticipating or postponing the switch. If postponing, we'd discuss it in the PR review.

Basically function registerType({ switchToModelVersionAt = '8.10.0', ...otherOptions }) {...}.

Given the short timeframe we are providing, I'm almost certain that some teams will need to postpone it.

But I don't think this is a blocker, so let's keep it as it is, and we can soften the validation in a follow-up PR if needed.

TinaHeiligers

We'll need to exclude types that are allowed to delay the switch over if that's what we agreed on but that can be done as a follow up.

TinaHeiligers

LGTM to CI green

…odel-version

kibana-ci · 2023-05-24T07:11:29Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 45e6183

Failed CI Steps

FTR Configs #5

Test Failures

[job] [logs] FTR Configs #5 / machine learning - data frame analytics classification saved search creation with filter and lucene query edits the analytics job and displays it correctly in the job list

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	85	86	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	399	403	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	479	483	+4
total			+6

History

💚 Build #129626 succeeded 7b375d0
💔 Build #129600 failed 6e391bb

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

…#158267) ## Summary Sets `switchToModelVersionAt` to `8.10.0` for all registered SO types, forcing them to switch to the model version API once `main` is on version `8.10.0` (~end of june)

set switchToModelVersionAt on all types during registration

ff4a1ad

pgayvallet added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// release_note:skip Skip the PR/issue when compiling release notes v8.9.0 labels May 23, 2023

pgayvallet added 2 commits May 23, 2023 15:52

add tests

6e391bb

updating snapshots

7b375d0

pgayvallet commented May 23, 2023

View reviewed changes

pgayvallet marked this pull request as ready for review May 23, 2023 15:08

pgayvallet requested a review from a team as a code owner May 23, 2023 15:08

jloleysens reviewed May 23, 2023

View reviewed changes

afharo reviewed May 23, 2023

View reviewed changes

TinaHeiligers reviewed May 23, 2023

View reviewed changes

TinaHeiligers self-requested a review May 23, 2023 16:15

TinaHeiligers approved these changes May 23, 2023

View reviewed changes

pgayvallet requested a review from rudolf May 23, 2023 17:41

pgayvallet added 2 commits May 24, 2023 07:27

Merge remote-tracking branch 'upstream/main' into kbn-xxx-switch-to-m…

6da7220

…odel-version

update tests

45e6183

pgayvallet enabled auto-merge (squash) May 24, 2023 06:02

afharo approved these changes May 24, 2023

View reviewed changes

pgayvallet merged commit acdadf2 into elastic:main May 24, 2023

kibanamachine added the backport:skip This PR does not require backporting label May 24, 2023

TinaHeiligers mentioned this pull request May 31, 2023

V2 migration algorithm: add tests for model versions #158697

Merged

		!Semver.valid(switchToModelVersionAt) \|\|
		Semver.gt(switchToModelVersionAt, globalSwitchToModelVersionAt)

Conversation

pgayvallet commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

pgayvallet May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet May 23, 2023

Choose a reason for hiding this comment

Uh oh!

pgayvallet May 23, 2023

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented May 23, 2023

Uh oh!

jloleysens May 23, 2023

Choose a reason for hiding this comment

Uh oh!

pgayvallet May 24, 2023

Choose a reason for hiding this comment

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

afharo May 23, 2023

Choose a reason for hiding this comment

Uh oh!

TinaHeiligers May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet May 23, 2023

Choose a reason for hiding this comment

Uh oh!

afharo May 24, 2023

Choose a reason for hiding this comment

Uh oh!

TinaHeiligers left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TinaHeiligers left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented May 24, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

API count

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pgayvallet commented May 23, 2023 •

edited

Loading

pgayvallet May 23, 2023 •

edited

Loading

TinaHeiligers May 23, 2023 •

edited

Loading

TinaHeiligers left a comment •

edited

Loading