Skip to content

Allow nested declaration for exposeToBrowser#128864

Merged
pgayvallet merged 4 commits intoelastic:mainfrom
pgayvallet:kbn-128794-nested-exposeToBrowser
Mar 30, 2022
Merged

Allow nested declaration for exposeToBrowser#128864
pgayvallet merged 4 commits intoelastic:mainfrom
pgayvallet:kbn-128794-nested-exposeToBrowser

Conversation

@pgayvallet
Copy link
Copy Markdown
Contributor

@pgayvallet pgayvallet commented Mar 30, 2022

Summary

Fix #128794

Allow to use a nested structure when defining PluginConfigDescriptor.exposeToBrowser to only expose a subset of a given configuration section/object.

const config = {
  schema: schema.object({
    monitoring: schema.object({
      credentials: schema.string(),
      ccs: schema.object({
        enabled: schema.boolean(),
      }),
      min_interval_seconds: schema.number(),
    })
  }),
  
  exposeToBrowser: {
    monitoring: {
      ccs: {
        enabled: true
      },
      min_interval_seconds: true
    }
  }
};

Checklist

@pgayvallet 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 Feature:Configuration Settings in kibana.yml v8.2.0 labels Mar 30, 2022
@pgayvallet pgayvallet marked this pull request as ready for review March 30, 2022 10:16
@pgayvallet pgayvallet requested a review from a team as a code owner March 30, 2022 10:16
@elasticmachine
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

LGTM! I just added a few nits for your consideration

)
)
),
this.configService
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mind the filter in line 225 (I couldn't comment there): since the root values do not necessarily are boolean now, are we OK with validating that a key simply exists?

Copy link
Copy Markdown
Contributor Author

@pgayvallet pgayvallet Mar 30, 2022

Choose a reason for hiding this comment

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

We're validating that at least one value exists (not a key), but that doesn't change much: Yea, I think we're fine.

Only edge case I see would be a scenario where the descriptor only defines nested keys with false values:

{
   nested: {
      key: false,
   }
}

this would 'bypass' the filtering. But the output of createBrowserConfig would then be an empty object regardless, so I think it's fine.

TBH we could probably get rid of this whole filter block and let createBrowserConfig generates empty configs for all plugins, but I wanted to avoid touching more code than necessary here.

Do you think it would make sense to get rid of this filter block in the current PR?

* @public
*/
export type ExposedToBrowserDescriptor<T> = {
[Key in keyof T]?: T[Key] extends Maybe<any[]>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: If we do Array<infer U> instead of any[] we remove the additional any and allow us to support cherry-picking properties in an array of objects if necessary 😇

const config = {
  myArray: [
    {
      propA: 'value',
      propB: 'value',
    }
  ]
};

const exposeToBrowser = {
  myArray: {
    propA: true
  }
}

Copy link
Copy Markdown
Contributor Author

@pgayvallet pgayvallet Mar 30, 2022

Choose a reason for hiding this comment

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

I 'stupidly' reproduced the type logic of MakeUsageFromSchema from the same file.

If we have a way to get rid of this any, I would be more than happy to. However regarding allowing to pick from the array, I'd rather hold that until we got a specific requirement for it, given the way we would be handling arrays in the exposeToBrowser definition would be slightly different and maybe a bit misleading (objectKey->properties, arrayKey->propertiesOfElements)

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id before after diff
core 2375 2376 +1

History

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

@klacabane
Copy link
Copy Markdown
Contributor

Added 7.17 and 8.1 labels as Stack Monitoring will need this to fix an issue in previous versions as well :)

@pgayvallet pgayvallet added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 30, 2022
@pgayvallet pgayvallet merged commit dd0a190 into elastic:main Mar 30, 2022
@kibanamachine
Copy link
Copy Markdown
Contributor

💔 All backports failed

Status Branch Result
7.17 Backport failed because of merge conflicts
8.1 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 128864

Questions ?

Please refer to the Backport tool documentation

pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Mar 30, 2022
* Allow nested declaration for `exposeToBrowser`

* update generated doc

* add utest

(cherry picked from commit dd0a190)

# Conflicts:
#	src/core/server/server.api.md
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Mar 30, 2022
* Allow nested declaration for `exposeToBrowser`

* update generated doc

* add utest

(cherry picked from commit dd0a190)

# Conflicts:
#	src/core/server/plugins/plugins_service.ts
#	src/core/server/server.api.md
pgayvallet added a commit that referenced this pull request Mar 30, 2022
* Allow nested declaration for `exposeToBrowser` (#128864)

* Allow nested declaration for `exposeToBrowser`

* update generated doc

* add utest

(cherry picked from commit dd0a190)

# Conflicts:
#	src/core/server/server.api.md

* update generated doc
pgayvallet added a commit that referenced this pull request Mar 30, 2022
)

* Allow nested declaration for `exposeToBrowser` (#128864)

* Allow nested declaration for `exposeToBrowser`

* update generated doc

* add utest

(cherry picked from commit dd0a190)

# Conflicts:
#	src/core/server/plugins/plugins_service.ts
#	src/core/server/server.api.md

* update generated odc
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 Feature:Configuration Settings in kibana.yml 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.17.2 v8.1.2 v8.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow exposeToBrowser nested configuration

6 participants