Skip to content

[Cloud Posture] add resource findings page#131325

Merged
orouz merged 5 commits intoelastic:mainfrom
orouz:resource_findings
May 3, 2022
Merged

[Cloud Posture] add resource findings page#131325
orouz merged 5 commits intoelastic:mainfrom
orouz:resource_findings

Conversation

@orouz
Copy link
Copy Markdown
Contributor

@orouz orouz commented May 2, 2022

Summary

this PR adds a new URL route to /findings/resource/:resourceId and its matching placeholder component.

we then use the resourceId param to:

  • show it in the page title
  • later on use to query elastic when populating the table

Demo

Screen.Recording.2022-05-02.at.16.31.19.mov

Checklist

Delete any items that are not applicable to this PR.

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cloudSecurityPosture 239 243 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
cloudSecurityPosture 389.2KB 391.5KB +2.3KB

History

  • 💔 Build #41758 failed 7058fe5726fbe24736f7943925093055a58584ea

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

@orouz orouz force-pushed the resource_findings branch from 370cb56 to 3a20df9 Compare May 3, 2022 09:20
@orouz orouz marked this pull request as ready for review May 3, 2022 11:11
@orouz orouz requested a review from a team as a code owner May 3, 2022 11:11
@orouz orouz added release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.3.0 labels May 3, 2022
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security Posture)

Copy link
Copy Markdown
Contributor

@kfirpeled kfirpeled left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@JordanSh JordanSh left a comment

Choose a reason for hiding this comment

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

see comment about the id translation, if it's intended please ask for a re-review


const BackToResourcesButton = () => {
return (
<Link to={generatePath(findingsNavigation.findings_by_resource.path)}>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we make this remember the page number you were in?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

currently it is out of scope of this task

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

not sure i follow, but generally - there are no page numbers in group-by-resource as we fetch based on after_key, not an index

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thought @JordanSh meant to save the state (in the url or other way)
but I see I might be wrong

<BackToResourcesButton />
<PageTitleText
title={
<div style={{ padding: euiTheme.size.s }}>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i think you used size.l on other pages

<FormattedMessage
id="xpack.csp.findings.resourceFindingsTitle"
defaultMessage="{resourceId} - Findings"
values={{ resourceId: params.resourceId }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this doesn't make much sense to me, do we actually need to translate ids? their purpose is usually to be consistent. unless there's a reason for this i would remove it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

id is not translated, however in different languages you would like to position the id differently. therefore you see the placeholder of the resource id in the text

@orouz orouz merged commit d4494ad into elastic:main May 3, 2022
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label May 3, 2022
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants