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.

Sign up for GitHub

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

Jump to bottom

[8.3] [Security Solution][Detections] Related Integrations & Required Fields Feedback & Fixes (#133050) #134148

Merged
merged 2 commits into from
Jun 10, 2022
Merged

[8.3] [Security Solution][Detections] Related Integrations & Required Fields Feedback & Fixes (#133050) #134148

merged 2 commits into from
Jun 10, 2022

Conversation

spong
Copy link
Member

@spong spong commented Jun 9, 2022

Backport

This will backport the following commits from main to 8.3:

Questions ?

Please refer to the Backport tool documentation

@spong
[Security Solution][Detections] Related Integrations & Required Field…
05b774a 
…s Feedback & Fixes (elastic#133050)

## Summary

Addressing the following feedback from elastic#132847:

- [X] On the Rule Management page package name is used instead of package title when the package has only 1 integration:
- [X] move integrations_popover to `related_integrations` directory
- [X] update useInstalledIntegrations to always return array of installedIntegration
- [X] move useInstalledIntegrations to `related_integrations` directory
- [X] Slight update to copy in Rules Table popover
- [X] Ok to use Rule Details UI within Rules Table popover content
- [X] Sort integrations alphabetically
- [X] Update left padding on version mis-match tooltip
- [X] elastic#133291
- [X] elastic#133269
- [X]  Add Kibana Advanced Setting for disabling integrations badge on Rules Table
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/171750790-ffa2e3ef-dd7a-499c-9b08-89bafc06dd50.png" />
</p>

- [ ]  Adds tests
  - [x] `useInstalledIntegrations` hook
  - [X] relatedIntegrations utils
  - [x] IntegrationDescription
- [ ]  Add loaders where necessary since there can now be API delay
  - May hold off on loaders as transition from no installed integrations -> installed integrations isn't too bad as-is

##### Updated integrations popover content on Rules Table to match Rule Details design
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172263941-3e948b41-7ef7-4281-8354-57e77ddeb433.png" />
</p>

In discussions with @banderror reviewing the different integration states (uninstalled, installed, enabled, and agents enrolled), we are now capturing the distinction between `Installed` and `Enabled` so that we don't confuse users when a package is installed but the integration isn't enabled/configured. I also added tooltips for clarifying each state and what action the user should perform to ensure compatibility. In collab with @yiyangliu9286 @jethr0null (comments below) -- we've consolidated to a single `Installed: enabled` badge, and updated `Uninstalled` to `Not installed` as well.

##### Tooltips

<details><summary>Not installed</summary>
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172264210-00064485-2df9-408e-953b-9294f16dedf2.png" />
</p>
</details>

<details><summary>Installed</summary>
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172263672-67b641cd-5895-464a-8897-f26bd0a61073.png" />
</p>
</details>

<details><summary>Installed: enabled</summary>
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172263783-563ea48d-c96c-4519-87b4-7076582f5da2.png" />
</p>
</details>

### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [X] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials
  - Collaborating with docs teams on this dedicated docs issue: elastic/security-docs#2015
- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
- [X] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))

(cherry picked from commit 7bfcb52)

# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/integrations_popover/helpers.tsx
#	x-pack/plugins/security_solution/public/common/components/integrations_popover/index.tsx
#	x-pack/plugins/security_solution/public/detections/components/rules/description_step/required_integrations_description.tsx
#	x-pack/plugins/security_solution/public/detections/components/rules/related_integrations/use_installed_integrations.tsx
@spong spong added the backport label Jun 9, 2022
@spong spong enabled auto-merge (squash) June 9, 2022 23:50
@spong
Copy link
Member Author

spong commented Jun 9, 2022

Note, the 1 file difference (24 here vs 23 in the original PR) is a result of the original PR picking up the move of x-pack/plugins/security_solution/public/detections/containers/detection_engine/rules/use_installed_integrations.tsx where-as this PR picked it up as a delete and add.

Manually confirmed diff and it remains the same as the original PR.

@spong
Copy link
Member Author

spong commented Jun 10, 2022

So apparently #132847 was merged right after the 8.3 branch was cut (not before) and so was never backported, which is why we needed to fix the missing type here in da62c11. I manually verified the remaining diff, and everything else looks 👍 since the remaining files were either moved or replaced, so this PR should resolve everything on the 8.3 branch.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #3 / Inspect Hosts stats and tables "before all" hook for "inspects the All Hosts Table"
  • [job] [logs] Security Solution Tests #3 / risk tab filters the table
  • [job] [logs] Security Solution Tests #3 / risk tab renders the table
  • [job] [logs] Security Solution Tests #3 / risk tab should be able to change items count per page

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3054 3060 +6

Async chunks

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

id before after diff
securitySolution 5.1MB 5.1MB +4.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 249.3KB 249.4KB +82.0B

History

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

@spong spong merged commit 9232094 into elastic:8.3 Jun 10, 2022
@spong spong deleted the backport/8.3/pr-133050 branch June 10, 2022 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
backport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@spong @kibana-ci