-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add more detailed unmet deps logging #70098
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let @dover to review this. Since it's under his expertise :)
Thanks @nickofthyme for doing this so quickly! ❤️
@joshdover I updated the existing tests for missing plugins. Let me know if you want any other tests to be added. |
], | ||
Array [ | ||
"Plugin \\"plugin-with-disabled-transitive-dep\\" has been disabled since some of its direct or transitive dependencies are missing or disabled.", | ||
"Plugin \\"plugin-with-disabled-transitive-dep\\" has been disabled since the following direct or transitive dependencies are missing or disabled: [\\"another-explicitly-disabled-plugin\\"]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a plugin to this test that has missing optional dependencies to validate that this log does not disable or log optional dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also added some for nested/indirect dependencies.
@@ -257,17 +264,28 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS | |||
pluginName: PluginName, | |||
pluginEnableStatuses: Map<PluginName, { plugin: PluginWrapper; isEnabled: boolean }>, | |||
parents: PluginName[] = [] | |||
): boolean { | |||
): { enabled: boolean; missingDependencies: string[] } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to change this return type to:
{ enabled: true } | { enabled: false; missingDependencies: string[] }
This helps us avoid bugs where we have a state that should be impossible, like:
{ enabled: true, missingDependencies: ['some-dep'] }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elasticmachine merge upstream |
@joshdover could you do one last pass on this, thanks! |
`Plugin "${pluginName}" has been disabled since the following direct or transitive dependencies are missing or disabled: ${JSON.stringify( | ||
pluginEnablement.missingDependencies | ||
)}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we generally avoid using JSON.stringify
and prefer using the .join
method:
`Plugin "${pluginName}" has been disabled since the following direct or transitive dependencies are missing or disabled: ${JSON.stringify( | |
pluginEnablement.missingDependencies | |
)}` | |
`Plugin "${pluginName}" has been disabled since the following direct or transitive dependencies are missing or disabled: [${pluginEnablement.missingDependencies.join(', ')}]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-oss-agent / Plugin Functional Tests.test/plugin_functional/test_suites/core_plugins/logging·ts.core plugins plugin logging writes info_pattern context to custom pattern appenderStandard Out
Stack Trace
Build metrics
History
To update your PR or re-run it, just comment with: |
Summary
Closes #54001
Adds log to existing log for unmet dependencies, to now include what unmet dependencies are missing.