-
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
[EuiProvider/Security Solution] Remove usage of deprecated modules for mounting React, Part II #182061
[EuiProvider/Security Solution] Remove usage of deprecated modules for mounting React, Part II #182061
Conversation
e1ff360
to
c7b6273
Compare
c7b6273
to
913db5e
Compare
78f6bbc
to
b22e6c6
Compare
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-threat-hunting-explore (Team:Threat Hunting:Explore) |
Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations) |
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
Pinging @elastic/security-detection-engine (Team:Detection Engine) |
Pinging @elastic/security-defend-workflows (Team:Defend Workflows) |
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.
Did a code review only. LGTM. I would wait for @szwarckonrad or @tomsonpl to verify osquery changes.
}: GetColumnProps): Array<EuiTableFieldDataColumnType<CspBenchmarkRulesWithStates>> => [ | ||
{ | ||
field: 'action', | ||
name: ( | ||
<EuiCheckbox | ||
id={RULES_ROW_SELECT_ALL_CURRENT_PAGE} | ||
checked={isCurrentPageRulesASubset(items, selectedRules) && isAllRulesSelectedThisPage} | ||
onChange={(e) => { | ||
onChange={(_e) => { |
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.
Should be safe to remove this unused param
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.
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.
DE changes LGTM
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.
desk tested and code LGTM for the Threat Hunting Investigations team!
…ii-threatintelligence
…ii-threatintelligence
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.
Thanks for the changes.
…ii-threatintelligence
…ii-threatintelligence
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.
Approved.
Tested on local environment.
1 non-blocking question
@@ -58,14 +58,15 @@ export const DetectionRuleCounter = ({ tags, createRuleFn }: DetectionRuleCounte | |||
}, [history]); | |||
|
|||
const createDetectionRuleOnClick = useCallback(async () => { | |||
const startServices = { analytics, notifications, i18n, theme }; |
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.
Newbie question: I thought these service were started when Kibana loads the plugins when it starts up since these are CoreStartup services.
Is validating the services have started?
If some of these services are the Notification service dependencies, should that service start it instead of passing them as params?
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.
Newbie question: I thought these service were started when Kibana loads the plugins when it starts up since these are CoreStartup services.
Is validating the services have started?
This is a good question.
- The services are started when Kibana loads up. My
startServices
variable is just a made-up name for a wrapper object of a few specific services that are available in CoreStart. - This PR focuses on updating the
toMountPoint
andKibanaRenderContextProvider
components, which are owned by SharedUX. They need theanalytics
,i18n
andtheme
services to be passed in. Rather than using a literal object of{ analytics, i18n, theme }
, I created a wrapper object, thinking that it's slightly less restrictive - more services could be needed for these components in the future.. - The previous code was only making the
notifications
service usable for downstream code. This change makes a set of services usable by creating thestartServices
wrapper object. - These changes aren't about validating that the services have started.
If you're wondering, how does calling useKibana().services
give us access to these services? They were stashed into a context in x-pack/plugins/cloud_security_posture/public/plugin.tsx
with <KibanaContextProvider services={{ ...core, ...plugins, storage }}>
. Therefore, everything nested under that provider can access these startup services from the context. Calling the hook to get the context saves us from having to pass down the services to every component through props.
If some of these services are the Notification service dependencies, should that service start it instead of passing them as params?
I take this to mean, couldn't we just call useKibana().services
to fetch the services from context where they're needed? I chose to stick with the existing coding style and pass them down from here. Also, React hooks can only be called from React components, and showCreateDetectionRuleSuccessToast
/ showChangeBenchmarkRuleStatesSuccessToast
are not React components.
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.
@seanrathier while going through the code to address your question, I found a few places that could be cleaned up: e941e01
…ii-threatintelligence
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: cc @tsullivan |
Summary
Partially addresses https://github.com/elastic/kibana-team/issues/805
These changes come up from searching in the code and finding where certain kinds of deprecated AppEx-SharedUX modules are imported. Reviewers: Please interact with critical paths through the UI components touched in this PR, ESPECIALLY in terms of testing dark mode and i18n.
This is the 2nd PR to focus on code within Security Solution, following #181099.
Note: this also makes inclusion of
i18n
andanalytics
dependencies consistent. Analytics is an optional dependency for the SharedUX modules, which wrapKibanaErrorBoundaryProvider
and is designed to capture telemetry about errors that are caught in the error boundary.Checklist
Delete any items that are not applicable to this PR.