Allow kbn-config-schema to ignore unknown keys by legrego · Pull Request #59560 · elastic/kibana

legrego · 2020-03-06T15:35:39Z

Summary

Updates @kbn/config-schema to allow the object type to accept, but remove unknown keys.

schema.object({}, { unknowns: 'ignore' })

This also removes the existing allowUnknowns property, and introduces an unknowns: 'allow' option in its place:

schema.object({}, { unknowns: 'allow' });

interface UnknownOptions {
  /**
   * Options for dealing with unknown keys:
   * - allow: unknown keys will be permitted
   * - ignore: unknown keys will not fail validation, but will be stripped out
   * - forbid (default): unknown keys will fail validation
   */
  unknowns?: 'allow' | 'ignore' | 'forbid';
}

See #58850 (comment) for a potential use case.

/cc @azasypkin

legrego · 2020-03-06T15:41:52Z

packages/kbn-config-schema/src/types/object_type.ts

+  ignoreUnknowns?: false;
+}
+// type check to not permit both to be set to `true` at the same time
+type UnknownOptions = AllowUnknowns | IgnoreUnknowns | ForbidUnknowns;


I did this because it was less invasive, but it feels messy to me.

Since these options are mutually exclusive of each other, I think a const might be better:

interface UnknownOptions { unknowns?: 'allow' | 'ignore' | 'forbid' }

with the default behavior being forbid.

I'm happy to make this change, but I wanted to discuss before doing so, as it'll touch a lot of downstream code.

It makes sense to me...even it will require to touch 70 places in the codebase 🙈 welcome to the platform team world.

elasticmachine · 2020-03-06T15:46:53Z

Pinging @elastic/kibana-platform (Team:Platform)

smith

APM changes look good to me. Thanks!

lizozom

@elastic/kibana-app-arch changes LGTM

weltenwort

infra changes LGTM

flash1293

Kibana app changes LGTM

legrego · 2020-03-09T13:44:08Z

@elasticmachine merge upstream

mshustov

Thanks!

jportner

See comment for minor nit, other than that LGTM!

jportner · 2020-03-16T15:50:15Z

src/core/server/http/router/route.ts

   *   validate: {
   *     // handler has access to raw non-validated params in runtime
-   *     params: schema.object({}, { allowUnknowns: true })
+   *     params: schema.object({}, { unknowns: 'allow' })


Somehow, the corresponding Markdown API doc (at docs/development/core/server/kibana-plugin-server.routeconfig.validate.md, line 52) doesn't reflect this change.

…nknown-keys

…into misc/ignore-unknown-keys

legrego · 2020-03-17T14:41:25Z

@elasticmachine merge upstream

kibanamachine · 2020-03-17T16:22:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 48563ae

History

💔 Build #33779 failed 9812671
💚 Build #31742 succeeded da9cf67
💚 Build #31558 succeeded c74167e
💚 Build #31454 succeeded 12a2cde

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

* allow kbn-config-schema to ignore unknown keys * Consolidate unknown key configuration * updates following merge Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358)

* allow kbn-config-schema to ignore unknown keys * Consolidate unknown key configuration * updates following merge Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: (51 commits) do not update cell background if is label cell (elastic#60308) FTR configurable test users (elastic#52431) [Reporting] Wholesale moves client to newest-platform (elastic#58945) [Ingest] Support `show_user` package registry flag (elastic#60338) [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358) [TSVB] fix text color when using custom background color (elastic#60261) Fix import to timefilter from in TSVB (elastic#60296) [NP] Get rid of usage redirectWhenMissing service (elastic#59777) ...

* alerting/view-in-app: (53 commits) fixed typo handle optional alerting plugin do not update cell background if is label cell (elastic#60308) FTR configurable test users (elastic#52431) [Reporting] Wholesale moves client to newest-platform (elastic#58945) [Ingest] Support `show_user` package registry flag (elastic#60338) [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358) [TSVB] fix text color when using custom background color (elastic#60261) ...

allow kbn-config-schema to ignore unknown keys

12a2cde

legrego commented Mar 6, 2020

View reviewed changes

legrego added enhancement New value added to drive a business result release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.7.0 v8.0.0 labels Mar 6, 2020

legrego marked this pull request as ready for review March 6, 2020 15:46

legrego requested a review from a team as a code owner March 6, 2020 15:46

Consolidate unknown key configuration

c74167e

legrego requested review from a team March 6, 2020 20:11

legrego requested review from a team as code owners March 6, 2020 20:11

smith approved these changes Mar 6, 2020

View reviewed changes

lizozom approved these changes Mar 8, 2020

View reviewed changes

weltenwort approved these changes Mar 9, 2020

View reviewed changes

flash1293 approved these changes Mar 9, 2020

View reviewed changes

Merge branch 'master' into misc/ignore-unknown-keys

da9cf67

mshustov approved these changes Mar 10, 2020

View reviewed changes

jportner approved these changes Mar 16, 2020

View reviewed changes

legrego added 3 commits March 17, 2020 08:41

Merge branch 'master' of github.com:elastic/kibana into misc/ignore-u…

87866d5

…nknown-keys

updates following merge

b0429dd

Merge branch 'misc/ignore-unknown-keys' of github.com:legrego/kibana …

9812671

…into misc/ignore-unknown-keys

Merge branch 'master' into misc/ignore-unknown-keys

48563ae

legrego merged commit 9318862 into elastic:master Mar 17, 2020

legrego deleted the misc/ignore-unknown-keys branch March 17, 2020 16:30

legrego mentioned this pull request Mar 17, 2020

[7.x] Allow kbn-config-schema to ignore unknown keys (#59560) #60407

Merged

Conversation

legrego commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

legrego Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

mshustov Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Mar 6, 2020

Uh oh!

smith left a comment

Choose a reason for hiding this comment

Uh oh!

lizozom left a comment

Choose a reason for hiding this comment

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

legrego commented Mar 9, 2020

Uh oh!

mshustov left a comment

Choose a reason for hiding this comment

Uh oh!

jportner left a comment

Choose a reason for hiding this comment

Uh oh!

jportner Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

legrego commented Mar 17, 2020

Uh oh!

kibanamachine commented Mar 17, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

legrego commented Mar 6, 2020 •

edited

Loading