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

Implement plugin config prefix #6554

Merged
merged 12 commits into from
Mar 23, 2016

Conversation

w33ble
Copy link
Contributor

@w33ble w33ble commented Mar 16, 2016

Closes #6304

Allows plugins to optionally prefix their config values via a new configPrefix parameter

When configPrefix is set, it uses that value for the config values. Otherwise it uses the default, which is simply the plugin id value.

Both of these are valid:

// without configPrefix
return new kibana.Plugin({
  id: 'myPlugin',
  config: function (Joi) {
    return Joi.object({
      mySetting: Joi.boolean().default(true),
    });
  },
  init: function (server, options) {
    const config = server.config();
    config.get('myPlugin.mySetting'); // returns true
  }
});
// with custom configPrefix
return new kibana.Plugin({
  id: 'myPlugin',
  configPrefix: 'customPluginPrefix',
  config: function (Joi) {
    return Joi.object({
      mySetting: Joi.boolean().default(true),
    });
  },
  init: function (server, options) {
    const config = server.config();
    config.get('customPluginPrefix.mySetting'); // returns true
  }
});

spalger and others added 4 commits March 16, 2016 15:30
- switch from maps to objects
  - use lodash's get/set/has
- use configPrefix instead of id, if set
- also adds an unset config method
if (initialVals) {
this.set(key, initialVals);
this[pendingSets].delete(key);
const path = toPath(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

path isn't used here

@spalger spalger assigned w33ble and unassigned spalger Mar 17, 2016
@w33ble w33ble assigned spalger and unassigned w33ble Mar 17, 2016
@spalger
Copy link
Contributor

spalger commented Mar 18, 2016

LGTM

@spalger spalger assigned w33ble and unassigned spalger Mar 18, 2016
this[schema] = Joi.object().keys(objKeys).default();
this[schema] = (function convertToSchema(children) {
let objKeys = Object.keys(children);
let schema = Joi.object().keys(objKeys).default();
Copy link
Contributor

Choose a reason for hiding this comment

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

passing objKeys here is unnecessary. Joi is itterating the own properties of objKeys and adding them as keys, but since it's an array it has no own properties and does nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I read the Joi docs wrong before.

The call to .keys() is required, otherwise the validation fails (and the tests do as a result), but it just needs to be given an empty object, since the actual keys are appended after the fact.

@w33ble
Copy link
Contributor Author

w33ble commented Mar 22, 2016

There appears to be what looks like a race condition here, where the keys in the schema are changing and then the validation fails.

I've used this to prefix the XPack settings with xpack, and I see the keys in getSchema/convertToSchema changing.

[ 'graph', 'monitoring', 'reporting' ]
...later
[ 'graph', 'monitoring' ]
...later still
[ 'graph', 'monitoring', 'shield' ]

And then Kibana fails due to schema issues: ValidationError: child "xpack" fails because ["reporting" is not allowed]

@w33ble
Copy link
Contributor Author

w33ble commented Mar 22, 2016

Issue was related to disabled plugins. Should be fixed now. @spalger I broke out the unset module and added tests, let me know if there's a scenario I missed in there though.

@w33ble w33ble assigned spalger and unassigned w33ble Mar 22, 2016
@w33ble
Copy link
Contributor Author

w33ble commented Mar 22, 2016

jenkins, test it

1 similar comment
@spalger
Copy link
Contributor

spalger commented Mar 22, 2016

jenkins, test it

@w33ble w33ble merged commit 4408069 into elastic:master Mar 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Plugin Configuration Prefix
3 participants