elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.67k stars 8.1k forks source link

Fail startup with unknown optionalPlugins dependency in dev mode #62937

Open timroes opened 4 years ago

timroes commented 4 years ago

Currently you can run Kibana completely fine if you specify an unknown/invalid optionalPlugins id in your kibana.json file. This make totally sense in production mode, but I'd suggest failing startup in dev mode for that.

Why? Plugin ids are not at all type-safe, meaning if we change the id of a plugin, it can easily be forgotten to update it in other plugins that just have this as optionalPlugins, since we might not even be aware of those other plugins, plus if a functional test for that specific feature is missing it will very easily slip all CI tests. (This e.g. happened here: https://github.com/elastic/kibana/pull/62543#issuecomment-610041905)

I think it would make sense to fail Kibana startup during dev mode if we specify an optionalPlugin id that doesn't exist. That way we could catch those errors directly during development, and also functional tests (which run in dev mode??) would fail and the CI wouldn't let it pass.

Alternatively if this solution doesn't sound feasible for reasons I've missed so far, could we have some kind of other CI check that validates all dependencies ids for their validity?

elasticmachine commented 4 years ago

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

pgayvallet commented 4 years ago

Why? Plugin ids are not at all type-safe, meaning if we change the id of a plugin, it can easily be forgotten to update it in other plugins that just have this as optionalPlugins, since we might not even be aware of those other plugins, plus if a functional test for that specific feature is missing it will very easily slip all CI tests

In an ideal world, this would be covered by FTR tests running a plugin and it's (optional) dependencies and asserting the dependencies-related features of the plugin are working as expected.

The https://github.com/elastic/kibana/pull/62543#issuecomment-610041905 reference you linked showed a test failing due to a pluginId reference being forgotten / not renamed, which seems to be what we want and how we should detect that imho.

I can understand that ATM every feature is not necessarily properly covered by FTR tests, and therefor this safety net could avoid a lot of developer errors during migration, but OTOH this would be introducing a significant behavioral difference between dev and prod environments.

Would like to see the other team members' opinion on that one.

and also functional tests (which run in dev mode??)

Someone would need to confirm that, but I think it's running in production mode (at least it should, as it does when FTR is running against could instances). I don't see any option setting the env to dev for serverArgs in test/common/config.js

timroes commented 4 years ago

But is there any legit use-case where we would want to have (inside the core Kibana repository) optional plugin dependencies to IDs that are not existing?

joshdover commented 4 years ago

I think it's really valuable to have little to no behavior differences between:

If we want to validate this for internal development, I suggest a script that runs on CI to read all the kibana.json files and verifies that the optional and required dependencies exist in the repository.

pgayvallet commented 4 years ago

If we want to validate this for internal development, I suggest a script that runs on CI to read all the kibana.json files and verifies that the optional and required dependencies exist in the repository.

That seems like a very reasonable solution. @timroes wdyt?

timroes commented 4 years ago

I agree with that. My really mainly want to fail CI. So as long as CI fails if we use it, I am fine :-)

pgayvallet commented 4 years ago

Should we also check for missing dependencies on test plugins (ie test/plugin_functional/plugins/*/kibana.json)?