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

Plugins: Add preInit capability #7069

Merged
merged 4 commits into from
Apr 27, 2016
Merged

Conversation

lukasolson
Copy link
Member

This PR adds the capability for a plugin to have a preInit function which is invoked prior to calling init on any of the other registered plugins.

One use case for this is to enable a plugin to programmatically override options that another plugin provides.

@@ -40,6 +40,10 @@ module.exports = async function (kbnServer, server, config) {
path.pop();
};

for (let plugin of plugins) {
await plugin.preInit();
Copy link
Contributor

@ycombinator ycombinator Apr 27, 2016

Choose a reason for hiding this comment

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

Does pre-initialization not require to follow the same plugin dependency graph as initialization does?

Copy link
Member Author

Choose a reason for hiding this comment

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

For the current use case, it doesn't, but I could see why that might be necessary. What are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally speaking, I'm against implementing something we don't need for a current use case (speculative generality is a code smell). In this case, however, implementing pre-initialization to parallel initialization would lend consistency to the codebase, and make it easier to reason about IMO.

@simianhacker
Copy link
Member

Looks reasonable to me.... I'd merge it

@ycombinator
Copy link
Contributor

Thanks @lukasolson. The change looks good. Would it be possible to add unit tests here to prove that all the pre-inits for a set of plugins are called before all the inits?

@lukasolson
Copy link
Member Author

I'm on it.

@lukasolson
Copy link
Member Author

jenkins, test it

@ycombinator
Copy link
Contributor

LGTM

@epixa
Copy link
Contributor

epixa commented Apr 27, 2016

jenkins, test it

@epixa epixa merged commit 4ff67e0 into elastic:master Apr 27, 2016
@lukasolson lukasolson deleted the plugin/preInit branch March 27, 2018 21:07
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.

5 participants