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

[Cases] Fix plugin lifecycle inconsistencies #177132

Merged
merged 11 commits into from
Mar 3, 2024

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Feb 17, 2024

Summary

The script from #171483 can identify inconsistencies and untyped dependencies in Kibana plugins. This PR fixes:

  • Move esUiShared, kibanaReact, kibanaUtils, and savedObjectsFinder to requiredBundles
  • Rename CasesUiSetup to CasesPublicSetup
  • Rename CasesUiStart to CasesPublicStart
  • Rename CasesSetup to CasesServerSetup
  • Rename CasesStart to CasesServerStart
  • Rename CasesPluginSetup (public) to CasesPublicSetupDependencies
  • Rename CasesPluginStart (public) to CasesPublicStartDependencies
  • Moved PluginsSetup from server/plugin.ts to server/types.ts and rename it to CasesServerSetupDependencies
  • Moved PluginsStart from server/plugin.ts to server/types.ts and rename it to CasesServerStartDependencies

I could not add the apm to optional plugins because I get an error due to circular dependencies. Observability and Secuirty solution have apm and cases as a dependency and probably this cause the following error.

Error
info Running model version mapping addition checks
   │ info Generating field lists from registry and file
   │ info Loading core with all plugins enabled so that we can get all savedObject mappings...
   │ERROR UNHANDLED ERROR: Error: Topological ordering of plugins did not complete, these plugins have cyclic or missing dependencies: ["aiAssistantManagementObservability","aiops","apm","assetManager","cases","cloudDefend","datasetQuality","elasticAssistant","enterpriseSearch","exploratoryView","infra","kubernetesSecurity","logsShared","logstash","metricsDataAccess","ml","monitoring","observability","observabilityAIAssistant","observabilityOnboarding","observabilityShared","observabilityLogsExplorer","osquery","profiling","securitySolution","securitySolutionEss","securitySolutionServerless","serverlessObservability","sessionView","synthetics","threatIntelligence","timelines","upgradeAssistant","uptime","ux"]
   │ERROR     at PluginsSystem.getTopologicallySortedPluginNames (plugins_system.ts:354:13)
   │ERROR     at PluginsSystem.uiPlugins (plugins_system.ts:274:36)
   │ERROR     at PluginsService.discover (plugins_service.ts:120:58)
   │ERROR     at Server.preboot (server.ts:172:30)
   │ERROR     at Root.preboot (index.ts:57:14)
   │ERROR     at extract_field_lists_from_plugins_worker.ts:51:3
ERROR UNHANDLED ERROR
ERROR Error: worker exited without sending mappings
          at extractFieldListsFromPlugins (extract_field_lists_from_plugins.ts:62:11)
          at processTicksAndRejections (node:internal/process/task_queues:95:5)
          at runModelVersionMappingAdditionsChecks (run_versions_mapping_additions_check.ts:28:66)
          at run_check_mappings_update_cli.ts:23:9
          at tooling_log.ts:84:18
          at description (run_check_mappings_update_cli.ts:22:7)
          at run.ts:73:10
          at withProcRunner (with_proc_runner.ts:29:5)
          at run (run.ts:71:5)

Also, the storage is passed by the cases plugin to the Kibana context and is not a dependency to another plugin. The script assumes that the storage is a plugin dependency and reports it. Neverthelss, this demonstrates that we should stop passing storage to the Kibana context and use hooks like useLocalStorage.

Lastly, ML uses cases but does not pass the licensing plugin to the KibanaContext. For that reason, I left it as optional in the Cases types. When #168178 is done we can change the type of the licensing service to required.

Results from the script

Screenshot 2024-02-17 at 8 54 32 AM

For maintainers

@cnasikas cnasikas added chore release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.14.0 labels Feb 17, 2024
@cnasikas cnasikas self-assigned this Feb 17, 2024
@cnasikas cnasikas force-pushed the fix_case_dependencies branch from bdb91e2 to 3c07cc0 Compare February 17, 2024 09:28
@cnasikas
Copy link
Member Author

/ci

@cnasikas
Copy link
Member Author

/ci

1 similar comment
@cnasikas
Copy link
Member Author

/ci

@cnasikas cnasikas marked this pull request as ready for review February 18, 2024 20:58
@cnasikas cnasikas requested review from a team as code owners February 18, 2024 20:58
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops-cases (Feature:Cases)

@cnasikas
Copy link
Member Author

/ci

@cnasikas cnasikas requested review from a team as code owners February 18, 2024 21:51
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

Copy link
Contributor

@adcoelho adcoelho left a comment

Choose a reason for hiding this comment

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

What about the storage dependency? It shows up on the print screen but you don't mention it in the description. Is it maybe part of kibanaUtils?

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

@cnasikas
Copy link
Member Author

cnasikas commented Feb 19, 2024

What about the storage dependency? It shows up on the print screen but you don't mention it in the description. Is it maybe part of kibanaUtils?

Good observation. I forgot about it. I added a paragraph about it.

@cnasikas
Copy link
Member Author

@elasticmachine merge upstream

@cnasikas
Copy link
Member Author

@elasticmachine merge upstream

@cnasikas cnasikas enabled auto-merge (squash) February 20, 2024 14:45
Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

@cnasikas cnasikas disabled auto-merge February 20, 2024 15:43
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Serverless Osquery Cypress Tests #3 / ALL - Saved queries Saved queries Complex Test should create a new query and verify:
  • hidden columns, full screen and sorting
  • pagination
  • query can viewed (status), edited and deleted should create a new query and verify:
  • hidden columns, full screen and sorting
  • pagination
  • query can viewed (status), edited and deleted
  • [job] [logs] Detection Engine - Exceptions - Security Solution Cypress Tests #3 / Exception list detail page Should edit list details Should edit list details

Metrics [docs]

✅ unchanged

History

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

cc @cnasikas

@cnasikas cnasikas merged commit c7f2a6e into elastic:main Mar 3, 2024
41 checks passed
@cnasikas cnasikas deleted the fix_case_dependencies branch March 3, 2024 14:47
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 3, 2024
semd pushed a commit to semd/kibana that referenced this pull request Mar 4, 2024
## Summary

The script from elastic#171483 can
identify inconsistencies and untyped dependencies in Kibana plugins.
This PR fixes:

- Move `esUiShared`, `kibanaReact`, `kibanaUtils`, and
`savedObjectsFinder` to `requiredBundles`
- Rename `CasesUiSetup` to `CasesPublicSetup`
- Rename `CasesUiStart` to `CasesPublicStart`
- Rename `CasesSetup` to `CasesServerSetup`
- Rename `CasesStart` to `CasesServerStart`
- Rename `CasesPluginSetup` (public) to `CasesPublicSetupDependencies`
- Rename `CasesPluginStart` (public) to `CasesPublicStartDependencies`
- Moved `PluginsSetup` from `server/plugin.ts` to `server/types.ts` and
rename it to `CasesServerSetupDependencies`
- Moved `PluginsStart` from `server/plugin.ts` to `server/types.ts` and
rename it to `CasesServerStartDependencies`

I could not add the `apm` to optional plugins because I get an error due
to circular dependencies. Observability and Secuirty solution have `apm`
and `cases` as a dependency and probably this cause the following error.

<details>
<summary>Error</summary>

```
info Running model version mapping addition checks
   │ info Generating field lists from registry and file
   │ info Loading core with all plugins enabled so that we can get all savedObject mappings...
   │ERROR UNHANDLED ERROR: Error: Topological ordering of plugins did not complete, these plugins have cyclic or missing dependencies: ["aiAssistantManagementObservability","aiops","apm","assetManager","cases","cloudDefend","datasetQuality","elasticAssistant","enterpriseSearch","exploratoryView","infra","kubernetesSecurity","logsShared","logstash","metricsDataAccess","ml","monitoring","observability","observabilityAIAssistant","observabilityOnboarding","observabilityShared","observabilityLogsExplorer","osquery","profiling","securitySolution","securitySolutionEss","securitySolutionServerless","serverlessObservability","sessionView","synthetics","threatIntelligence","timelines","upgradeAssistant","uptime","ux"]
   │ERROR     at PluginsSystem.getTopologicallySortedPluginNames (plugins_system.ts:354:13)
   │ERROR     at PluginsSystem.uiPlugins (plugins_system.ts:274:36)
   │ERROR     at PluginsService.discover (plugins_service.ts:120:58)
   │ERROR     at Server.preboot (server.ts:172:30)
   │ERROR     at Root.preboot (index.ts:57:14)
   │ERROR     at extract_field_lists_from_plugins_worker.ts:51:3
ERROR UNHANDLED ERROR
ERROR Error: worker exited without sending mappings
          at extractFieldListsFromPlugins (extract_field_lists_from_plugins.ts:62:11)
          at processTicksAndRejections (node:internal/process/task_queues:95:5)
          at runModelVersionMappingAdditionsChecks (run_versions_mapping_additions_check.ts:28:66)
          at run_check_mappings_update_cli.ts:23:9
          at tooling_log.ts:84:18
          at description (run_check_mappings_update_cli.ts:22:7)
          at run.ts:73:10
          at withProcRunner (with_proc_runner.ts:29:5)
          at run (run.ts:71:5)
```

</details>

Also, the `storage` is passed by the `cases` plugin to the Kibana
context and is not a dependency to another plugin. The script assumes
that the `storage` is a plugin dependency and reports it. Neverthelss,
this demonstrates that we should stop passing `storage` to the Kibana
context and use hooks like `useLocalStorage`.

## Results from the script

<img width="727" alt="Screenshot 2024-02-17 at 8 54 32 AM"
src="https://github.com/elastic/kibana/assets/7871006/1c86f501-72c5-473c-92f5-7a4935da914b">


### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Kibana Machine <[email protected]>
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
## Summary

The script from elastic#171483 can
identify inconsistencies and untyped dependencies in Kibana plugins.
This PR fixes:

- Move `esUiShared`, `kibanaReact`, `kibanaUtils`, and
`savedObjectsFinder` to `requiredBundles`
- Rename `CasesUiSetup` to `CasesPublicSetup`
- Rename `CasesUiStart` to `CasesPublicStart`
- Rename `CasesSetup` to `CasesServerSetup`
- Rename `CasesStart` to `CasesServerStart`
- Rename `CasesPluginSetup` (public) to `CasesPublicSetupDependencies`
- Rename `CasesPluginStart` (public) to `CasesPublicStartDependencies`
- Moved `PluginsSetup` from `server/plugin.ts` to `server/types.ts` and
rename it to `CasesServerSetupDependencies`
- Moved `PluginsStart` from `server/plugin.ts` to `server/types.ts` and
rename it to `CasesServerStartDependencies`

I could not add the `apm` to optional plugins because I get an error due
to circular dependencies. Observability and Secuirty solution have `apm`
and `cases` as a dependency and probably this cause the following error.

<details>
<summary>Error</summary>

```
info Running model version mapping addition checks
   │ info Generating field lists from registry and file
   │ info Loading core with all plugins enabled so that we can get all savedObject mappings...
   │ERROR UNHANDLED ERROR: Error: Topological ordering of plugins did not complete, these plugins have cyclic or missing dependencies: ["aiAssistantManagementObservability","aiops","apm","assetManager","cases","cloudDefend","datasetQuality","elasticAssistant","enterpriseSearch","exploratoryView","infra","kubernetesSecurity","logsShared","logstash","metricsDataAccess","ml","monitoring","observability","observabilityAIAssistant","observabilityOnboarding","observabilityShared","observabilityLogsExplorer","osquery","profiling","securitySolution","securitySolutionEss","securitySolutionServerless","serverlessObservability","sessionView","synthetics","threatIntelligence","timelines","upgradeAssistant","uptime","ux"]
   │ERROR     at PluginsSystem.getTopologicallySortedPluginNames (plugins_system.ts:354:13)
   │ERROR     at PluginsSystem.uiPlugins (plugins_system.ts:274:36)
   │ERROR     at PluginsService.discover (plugins_service.ts:120:58)
   │ERROR     at Server.preboot (server.ts:172:30)
   │ERROR     at Root.preboot (index.ts:57:14)
   │ERROR     at extract_field_lists_from_plugins_worker.ts:51:3
ERROR UNHANDLED ERROR
ERROR Error: worker exited without sending mappings
          at extractFieldListsFromPlugins (extract_field_lists_from_plugins.ts:62:11)
          at processTicksAndRejections (node:internal/process/task_queues:95:5)
          at runModelVersionMappingAdditionsChecks (run_versions_mapping_additions_check.ts:28:66)
          at run_check_mappings_update_cli.ts:23:9
          at tooling_log.ts:84:18
          at description (run_check_mappings_update_cli.ts:22:7)
          at run.ts:73:10
          at withProcRunner (with_proc_runner.ts:29:5)
          at run (run.ts:71:5)
```

</details>

Also, the `storage` is passed by the `cases` plugin to the Kibana
context and is not a dependency to another plugin. The script assumes
that the `storage` is a plugin dependency and reports it. Neverthelss,
this demonstrates that we should stop passing `storage` to the Kibana
context and use hooks like `useLocalStorage`.

## Results from the script

<img width="727" alt="Screenshot 2024-02-17 at 8 54 32 AM"
src="https://github.com/elastic/kibana/assets/7871006/1c86f501-72c5-473c-92f5-7a4935da914b">


### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting chore Feature:Cases Cases feature release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:obs-ux-management Observability Management User Experience Team Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.