-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[9.0][Security Assistant] Add space id provider #225033
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
Conversation
| jest.mock('../../common/lib/kibana', () => ({ | ||
| useKibana: jest.fn(), | ||
| })); | ||
| jest.mock('../../common/hooks/use_space_id', () => ({ |
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.
Is it possible to modify this test file so that it does not mock AssistantSettingsManagement? Alternatively, it would be great if we could add a test that would fail if is removed by accident in the future.
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 guess we could try to do that by wrapping assistant settings management component into the AssistantSpaceIdProvider within the plugin setup if we have access to active space id from there https://github.com/elastic/kibana/blob/main/x-pack/solutions/security/plugins/security_solution/public/plugin.tsx#L214. This way we would avoid using useSpaceId hook completely.
But if that is too much work (+ I guess this is an urgent fix), I think we can also go with the current solution and update it later if needed.
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.
In the implementation on the main branch:
AssistantSpaceIdProvider was introduced but was accidentally dropped during its 9.0 backport
x-pack/solutions/security/plugins/security_solution/public/assistant/stack_management/management_settings.tsx:
I applied the same fix for its test file but some tests failed, I'm still trying to figure out:
x-pack/solutions/security/plugins/security_solution/public/assistant/stack_management/management_settings.test.tsx:
https://github.com/elastic/kibana/pull/221566/files#diff-14290c240aafc6f614c221dbde3b575f627f0ab8484b7186ed4d8e029fc9c3b7
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.
Sounds good! Thank you for the fix - tested locally and it worked. Will leave approval to @KDKHD
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.
Found another line dropped during the backport: https://github.com/elastic/kibana/pull/221566/files#diff-09e012f07b9ef5bd3559d3e05261436872407428a0fa8d1e2b4651b8f887cabcR16
I'd keep AssistantSettingsManagement to align with the code with the main branch:
https://github.com/elastic/kibana/pull/221566/files#diff-14290c240aafc6f614c221dbde3b575f627f0ab8484b7186ed4d8e029fc9c3b7
and added scenarios for both space id is available and unavailable.
I'll also re-think the error display for the AssistantSpaceIdProvider and try not to let it blocks the UI (I'll do the enhancement in a new PR and do the code change from the main branch).
...security/plugins/security_solution/public/assistant/stack_management/management_settings.tsx
Outdated
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Async chunks
|
Summary
#224127
Steps to reproduce: (env)
Expected: https://www.elastic.co/docs/solutions/security/ai/ai-assistant-knowledge-base#_option_2_enable_knowledge_base_from_the_security_ai_settings
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.