Skip to content

allows plugins to define validation schema for "enabled" flag#50286

Merged
mshustov merged 4 commits intoelastic:masterfrom
mshustov:issue-49289-read-schema-for-enabled
Nov 18, 2019
Merged

allows plugins to define validation schema for "enabled" flag#50286
mshustov merged 4 commits intoelastic:masterfrom
mshustov:issue-49289-read-schema-for-enabled

Conversation

@mshustov
Copy link
Copy Markdown
Contributor

@mshustov mshustov commented Nov 12, 2019 •
edited
Loading

Summary

the first commit closes: #49012
validation error message gives a hint about source of the error: "[config validation of [key]]:...

the second commit closes: #49289
plugins are able to define config schema with default value for enabled flag

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

Dev docs

If you want your plugin to be disabled by default you can specify it via config:

export const config = {
  schema: schema.object({ enabled: schema.boolean({ defaultValue: true }) })
}

@mshustov mshustov added review chore Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.6.0 labels Nov 12, 2019
@mshustov mshustov requested a review from a team as a code owner November 12, 2019 10:16
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Nov 12, 2019

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

mshustov
mshustov commented Nov 12, 2019
View reviewed changes
src/core/server/config/config_service.ts
if (!config.has(enabledPath)) {

// if plugin hasn't got a config schema, we try to read "enabled" directly
const isEnabled =
Copy link
Copy Markdown
Contributor Author

@mshustov mshustov Nov 12, 2019

Choose a reason for hiding this comment

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

As an alternative solution, we can always extend the plugin config with a schema for the enabled flag. Although, I'd prefer to do not touch the user schema.

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Nov 12, 2019

💔 Build Failed

@paul-tavares
Copy link
Copy Markdown
Contributor

paul-tavares commented Nov 13, 2019

Hi @restrry
I tried this out for the Endpoint Security plugin and it works well - thank you for working on it.
Do you know where I can find more information about config (what's available and how to use it)?

@mshustov
Copy link
Copy Markdown
Contributor Author

mshustov commented Nov 14, 2019

@paul-tavares right now you can only specify config schema. More details about config https://github.com/restrry/kibana/blob/b9a43a1788486c0d064cbe7aaff1fc7e987f7dfa/src/core/MIGRATION.md#configure-plugin
A schema is defined with @kbn/config-schema package. Docs for the package are in progress #50307
Let me know if you see something missed in the docs.

@mshustov
Copy link
Copy Markdown
Contributor Author

mshustov commented Nov 14, 2019

@elasticmachine merge upstream

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

@paul-tavares
Copy link
Copy Markdown
Contributor

paul-tavares commented Nov 14, 2019

@restrry thanks for that. I would not have thought of looking in the Migration docs because we're a brand new plugin that will be using only the new platform - but we're probably the exception. I will make a mental note to also review migration type of docs when looking for info. 😄

Re: Docs
I did not see an area that detail what configuration attributes are available from the platform side - for example: enabled. Knowing that list would help to ensure that plugins don't try to use "reserved" settings. The example does provide some other values, but not clear if those are used by the platform or if specific to the plugin.

One observation I would like to share, which perhaps is more specific to the disablement of a plugin, is the config is added to the server/index, which causes the actual sever code to be loaded (require()). Although the exported plugin function is never called, depending on what the plugin server code does that might acutely cause it to "run" (example: any type of setup outside of Plugin setup, start and stop lifecycle hooks)

@oatkiller oatkiller mentioned this pull request Nov 15, 2019
Closed
9 tasks
@mshustov
Copy link
Copy Markdown
Contributor Author

mshustov commented Nov 18, 2019

@elasticmachine merge upstream

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Nov 18, 2019

💚 Build Succeeded

@mshustov mshustov merged commit 583500e into elastic:master Nov 18, 2019
@mshustov mshustov deleted the issue-49289-read-schema-for-enabled branch November 18, 2019 09:25
@mshustov mshustov mentioned this pull request Nov 18, 2019
Merged
mshustov added a commit to mshustov/kibana that referenced this pull request Nov 18, 2019
@mshustov
allows plugins to define validation schema for "enabled" flag (elasti…
f6b7c05 
…c#50286)

* validation error message gives a hint about error source

* allows plugins to define validation schema for "enabled" flag
@mshustov mshustov removed the review label Nov 18, 2019
mshustov added a commit that referenced this pull request Nov 18, 2019
@mshustov
allows plugins to define validation schema for "enabled" flag (#50286) (
a2f71f6 
#50885)

* validation error message gives a hint about error source

* allows plugins to define validation schema for "enabled" flag
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 18, 2019
@jloleysens
Merge remote-tracking branch 'upstream/master' into np-migration/watc…
58b7d0c 
…her [skip ci]

* upstream/master: (54 commits)
  allows plugins to define validation schema for "enabled" flag (elastic#50286)
  Add retry to find.existsByDisplayedByCssSelector (elastic#48734)
  [i18n] integrate latest translations (elastic#50864)
  ui/resize_checker 👉 src/plugins/kibana_utils (elastic#44750)
  Fix @reach/router types (elastic#50863)
  [ML] Adding ML node warning to overview and analytics pages (elastic#50766)
  Bump storybook dependencies (elastic#50752)
  [APM Replace usage of idx with optional chaining (elastic#50849)
  [SIEM] Fix eslint errors (elastic#49713)
  Improve "Browser client is out of date" error message (elastic#50296)
  [SIEM][Detection Engine] REST API improvements and changes from UI/UX feedback (elastic#50797)
  Move @kbn/es-query into data plugin - es-query folder (elastic#50182)
  Index Management new platform migration (elastic#49359)
  Increase retry for cloud snapshot to finish (elastic#50781)
  Removing EuiCode from inside EuiPanel (elastic#50683)
  [SIEM] Tests for search_after and bulk index (elastic#50129)
  Make babel understand TypeScript 3.7 syntax (elastic#50772)
  Fixing mocha tests and broken password change status codes (elastic#50704)
  [Canvas] Use compressed forms in sidebar (elastic#49419)
  Add labels to shell scripts in Jenkins (elastic#49657)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pgayvallet pgayvallet pgayvallet approved these changes

Assignees

No one assigned

Labels

backported chore Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.6.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Plugins can declare disability be default Better reporting of config errors

5 participants

@mshustov @elasticmachine @paul-tavares @pgayvallet @joshdover