Skip to content

Ftrs restructure files#175417

Merged
yctercero merged 13 commits intoelastic:mainfrom
yctercero:ftrs-restructure-files
Jan 26, 2024
Merged

Ftrs restructure files#175417
yctercero merged 13 commits intoelastic:mainfrom
yctercero:ftrs-restructure-files

Conversation

@yctercero
Copy link
Contributor

@yctercero yctercero commented Jan 24, 2024

Summary

While it touches a large number of files, this PR is purely folder restructure. It is not intended to change any test functionality or enabled/disable any tests. Updates the D&R FTRs to follow a similar domain based folder structure like that of cypress. This should greatly simplify code owners and also allow us to quickly understand different licensing coverage by looking at the folders.

New folder structure

Screenshot 2024-01-25 at 5 18 45 PM

Example of when there are tests for multiple licenses

Screenshot 2024-01-03 at 5 52 37 PM

Open questions

  • @elastic/security-detection-rule-management owns the rule patch/update routes, but we own the edit UI? Should those routes be moved in under rules management?

@yctercero
Copy link
Contributor Author

/ci

@WafaaNasr
Copy link
Contributor

/ci

@WafaaNasr
Copy link
Contributor

/ci

@yctercero
Copy link
Contributor Author

/ci

@yctercero yctercero marked this pull request as ready for review January 25, 2024 07:02
@yctercero yctercero requested review from a team as code owners January 25, 2024 07:02
@yctercero yctercero self-assigned this Jan 25, 2024
@yctercero yctercero added release_note:skip Skip the PR/issue when compiling release notes Team:Detection Engine Security Solution Detection Engine Area v8.13.0 labels Jan 25, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-engine (Team:Detection Engine)

Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

New entity analytics folder structure LGTM!

Thank you, Yara!

timeout_in_minutes: 120
retry:
automatic:
- exit_status: '1'
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't want to block, makes more sense as a follow up for the whole file:

It looks like we don't have any retries for spot instance preemptions. The exit status is -1 in that case. Depending on stability requirements we can swap out spot instances or add retries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MadameSheema any feedback on what the suggested value is?

Copy link
Contributor

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

.buildkite, ftr_configs.yml

Copy link
Contributor

@JDKurma JDKurma 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
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

I had some nits about whitespace and test names, but the general idea here is very straightforward and well-contained. I also perused the CODEOWNERS changes, and didn't find anything odd or out of place.

LGTM, thanks!

Copy link
Contributor

@nikitaindik nikitaindik left a comment

Choose a reason for hiding this comment

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

Thanks for reorganizing it @yctercero! Codeowner assignments look much cleaner now!

I've reviewed the rule management part. In my opinion, it's reasonable to move the patch and the update endpoints under rules management since RM owns these. Otherwise we'll need to carve out an exception for these in the codeowners file.


One thing that caught my attention is this nested "rule_management" dir at:
x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_management

A dir named "rule_management" that sits inside "rules_management" makes navigation a bit confusing. Also, the name is very generic for a domain name, which may promote dumping all kinds of tests there. I think it might make more sense to move the tests contained in the inner "rules_management" dir to the domain level above, like:
Scherm­afbeelding 2024-01-25 om 21 58 39

I'm sure Georgii has a strong opinion about this :), so it warrants a team discussion once he returns. Furthermore, it's out of the scope of this PR, so I approve.

@yctercero yctercero enabled auto-merge (squash) January 26, 2024 07:21
@kibana-ci
Copy link

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @yctercero

@yctercero yctercero merged commit e2a9470 into elastic:main Jan 26, 2024
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Jan 26, 2024
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
## Summary

While it touches a large number of files, this PR is purely folder
restructure. It is not intended to change any test functionality or
enabled/disable any tests. Updates the D&R FTRs to follow a similar
domain based folder structure like that of cypress. This should greatly
simplify code owners and also allow us to quickly understand different
licensing coverage by looking at the folders.

Co-authored-by: Wafaa Nasr <wafaa.nasr@elastic.co>
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:Detection Engine Security Solution Detection Engine Area v8.13.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.