[Security Solution][Expandable flyout] - replace advanced settings with feature flag#184169
Conversation
0635f52 to
83f1baa
Compare
83f1baa to
3dcb99e
Compare
|
Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations) |
Files by Code Ownerelastic/appex-sharedux
elastic/kibana-core
elastic/kibana-localization
elastic/kibana-management
elastic/kibana-telemetry
elastic/security-defend-workflows
elastic/security-detection-engine
elastic/security-engineering-productivity
elastic/security-entity-analytics
elastic/security-solution
elastic/security-threat-hunting-explore
elastic/security-threat-hunting-investigations
|
rylnd
left a comment
There was a problem hiding this comment.
Detection engine changes overall LGTM.
I had one question about a perceived loss in test coverage, but I'm approving on the assumption that it was a conscious decision. If not, it should be easy to add back the removed test.
There was a problem hiding this comment.
The addition of the Table tab test is good, but could you explain why the JSON view coverage was removed from cypress?
There was a problem hiding this comment.
hey @rylnd, I can explain for sure!
I tried for 2+ hours to fix the JSON test, but the issue is the new flyout uses the JsonCodeEditor from the unified_doc_viewer which is different from the old flyout that was using a simple EuiCodeBlock. The code block was allowing us to easily retrieve the content and test against it. The JsonCodeEditor does not allow to do that.
The only way I kinda found to get it to work was to Cypress scroll to the correct position and select the row, but this is extremely flaky and we should never do that.
My thought was, for this test we don't really care if the value is showing in the JSON tab. The purpose of this test is not to verify that the flyout JSON tab shows the correct value. We should trust that the JSON tab and its components are rendering the document correctly (and we already have tests for that on the expandable flyout section). What we care about if the fact that the document has the correct value. The easiest way I found to do this was by filtering the table and verifying that the expected value was there.
Is that a wrong assumption?
christineweng
left a comment
There was a problem hiding this comment.
Desk tested with the flag on and off and LGTM! Thanks for cleaning up all the cypress tests!
There was a problem hiding this comment.
nit for readability, consider rephrasing it to "should disable expandable flyout when expandableFlyoutDisabled flag is true"
There was a problem hiding this comment.
for backlog: we should have some cypress tests for flyout in timeline - an expandable flyout equivalent of this file
There was a problem hiding this comment.
yeah, I was being lazy... I added some basic tests (here 7f60145) that we should probably improve a bit when we remove this entire old flyout codebase!
3dcb99e to
f4f61bc
Compare
There was a problem hiding this comment.
this test highlighted a bug with how the expandable flyout is set up to work in Cases. As the expandable flyout has been enabled since 8.10, this PR doesn't introduce any new issues, but just reveals the fact that the Cypress tests were behind, by still using the old flyout.
We'll take a look at the ticket to fix the issue shortly, and will re-enable the tests!
There was a problem hiding this comment.
just fyi, created ticket for our team to re-enable this: https://github.com/elastic/security-team/issues/9626
gergoabraham
left a comment
There was a problem hiding this comment.
@elastic/security-defend-workflows related changes look good! thanks for cleaning up the tests! 👐
There was a problem hiding this comment.
no, I had skipped it because my branch was opened at the time it was failing and you were working on the fix. It's unskipped in my local, and working!
I'm fixing a couple of other comments then will push a commit. No changes to this file anymore!
f4f61bc to
7f60145
Compare
7f60145 to
5276ab1
Compare
- unskip tour cypress test - update investigate in timeline cypress test - fix test title
5276ab1 to
36ae0a0
Compare
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
## Summary MKI environment does not support feature flags. The test needs the old flyout to work, but the new flyout is now rendered by default. PR Athat introduced the issue: #184169 ### Checklist - [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
Summary
The goal of this PR is to simplify the current expandable flyout advanced settings and feature flag setup. We currently have 1 advanced settings and 5 feature flags around the usage of the expandable flyout in Security Solution. The advanced settings and all the feature flags are
trueby default.This PR consolidates the 5 feature flags and the advanced settings into a single feature flag. The new flag is called
expandableFlyoutDisabledand is set tofalseby default. That way customers can turn the usage of the expandable flyout within Security Solution with one change in theirkibana.ymlfile, should the need arise.Here are the main changes:
securitySolution:enableExpandableFlyoutadvanced settings, so that users cannot revert to the old flyout through the UIexpandableEventFlyoutEnabled,expandableFlyoutInCreateRuleEnabled,expandableTimelineFlyoutEnabled,newUserDetailsFlyoutandnewHostDetailsFlyoutfeature flags. All these flags weretrueby default so we're not expecting customers to change these values.TO DO
https://github.com/elastic/security-team/issues/9360