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

Fix plugin enabled config options #60998

Merged
merged 4 commits into from
Mar 25, 2020

Conversation

joshdover
Copy link
Contributor

Summary

Fixes #58713

This PR does two things:

  • Adds the enabled key to any plugin config schemas that are missing it and must have it
  • Fixes the configPath for any NP plugins that either didn't match their existing legacy configPrefix or the previous prefix they had prior to migration.
    • Note, there are some xpack plugins that do not have the xpack prefix. This is to maintain BWC with their previously defined settings such as translations and console_extensions. In a future change, these could be renamed using the deprecations framework, however in this PR I just wanted to get things in a BWC state.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:fix Feature:New Platform v7.7.0 labels Mar 23, 2020
@joshdover joshdover requested a review from a team as a code owner March 23, 2020 21:32
@joshdover joshdover requested a review from a team March 23, 2020 21:32
@joshdover joshdover requested a review from a team as a code owner March 23, 2020 21:32
@joshdover joshdover requested a review from a team March 23, 2020 21:32
@joshdover joshdover requested review from a team as code owners March 23, 2020 21:32
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot added the Feature:Drilldowns Embeddable panel Drilldowns label Mar 23, 2020
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Alerting code LGTM

Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

should we add a warning or even throw an error if schema presents, but schema.enabled is not defined?

@pgayvallet
Copy link
Contributor

should we add a warning or even throw an error if schema presents, but schema.enabled is not defined?

Maybe a warning. But if we want that to be mandatory, I think we should just add it when missing instead of throwing an error?

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

observability changes seem trivial 👍

@joshdover
Copy link
Contributor Author

should we add a warning or even throw an error if schema presents, but schema.enabled is not defined?

Maybe a warning. But if we want that to be mandatory, I think we should just add it when missing instead of throwing an error?

I don't think an exception is the right idea. There are some plugins that I didn't add the enabled key to:

  • licensing
  • usage_collection
  • kibana_legacy
  • home

I choose not to add these for two main reasons:

  • There are plugins that have these listed as a required dependency (when maybe they shouldn't, especially home); and/or
  • Config keys for disabling these plugins did not currently exist AND there wasn't a good reason to add one right now

Since I'm just trying to get this in to 7.7 to maintain BWC in our config, I don't think we need to add these now. I would accept that maybe we should require the ability to disable of all of these plugins, but that will require more changes than I'm comfortable with for this PR.

I can do that in a follow up though!

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

app arch changes LGTM

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Kibana App code looks good to me.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@joshdover joshdover merged commit 37c92ca into elastic:master Mar 25, 2020
@joshdover joshdover deleted the np/fix-enabled-config branch March 25, 2020 21:09
joshdover added a commit to joshdover/kibana that referenced this pull request Mar 25, 2020
joshdover added a commit to joshdover/kibana that referenced this pull request Mar 25, 2020
joshdover added a commit that referenced this pull request Mar 26, 2020
* Fix plugin enabled config options (#60998)

* Fix types
joshdover added a commit that referenced this pull request Mar 26, 2020
* Fix plugin enabled config options (#60998)

* Fix types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Drilldowns Embeddable panel Drilldowns Feature:New Platform release_note:fix Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Whitelist 'enabled' config for Platform plugins