[Security Solution][Serverless] Github tickets / notifications by dkirchan · Pull Request #197265 · elastic/kibana

dkirchan · 2024-10-22T15:01:58Z

Summary

This PR is accompanied with the PR in kibana-operations repo and integrates the security solution quality gate pipelines with the functionality to create ticket when a test fails and then notify the respective codeowner team with a Test failed alert. Also when a test fails a second time and a ticket exist a new comment is added in the same issue that the test failed again in a given pipeline.

Last a similar flow exist when a test is skipped by a team.

Specifications:

In the failed_test_reporter_cli.ts a new field is introduced prependTitle which is initialized by the environmental variable process.env.PREPEND_FAILURE_TITLE if exists, in order to add a custom text of preference before the issue title.

The scope of this is to be able to give the team the opportunity to add some tags or any convention agreed within the team in the issue created in order to easily understand the context without opening the ticket. If this field is not initialized, the normal existing flow proceeds. Here an example of the prependTitle usage can be seen where the tags [MKI][QA] are prepended in order to identify where did the test fail, having the same exactly tests running on both CI and MKI. This means that a github issue with the exact same title would be created for both cases if this prepend title field would not exist.

In the junit_transformer/lib.ts a new functionality is introduced to cover an existing issue. The existing issue is: the fact that we are retrying the failed test in cypress in the parallel_serverless script, leads to two junit xml result files completely identical with only difference the execution time and the timestamps. This change will take one by one the xml outputs, transform them exactly as it was happening before and then save the file, but also remove the time and timestamp fields, convert it to a string and add it to a "state" list. Then in a next file if it is already parsed and saved to the list having the exact same results within, it deletes the file instead of saving it transformed.

The problem before this fix was the fact that when two xml outputs existed, a github ticket was created and when parsing-uploading the second file it was immediately failing for a second time as well. This means that new-failure notification was never triggered after the github actions flow split, and the existing failure was always triggered, something that most teams have disabled.

With the fix, the identical files are parsed but only once uploaded and the new failure flow works again.

elasticmachine · 2024-10-22T15:02:01Z

Pinging @elastic/security-solution (Team: SecuritySolution)

wayneseymour · 2024-10-22T15:24:36Z

x-pack/plugins/security_solution/scripts/junit_transformer/lib.ts

+// Function to remove specific fields from an XML object in order to
+// compare them as strings.
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function removeFields(obj: any, fieldsToRemove: string[]): any {


any reason for not having an explicit test for this function?

Checking over the tests of the whole package junit_transformer it looks like it is not expicitly tested as it relates to test code. I guess its not needed to try to keep a non existing test coverage. Also this part of code is tested by the generic test updates the file in place, applying the expected transformation as all of the rest of the code.

Ok, I'd prefer an explicit test, but you sound confident enough in your response to me.
Thanks.

Oh, one last thing, I was not referring to coverage, but to my own personal nits about testing functions explicitly.
From your comment, it sounds like removeFields is only tested implicitly.
As this lib.ts file is not actually owned by my team, no worries.

wayneseymour

CR only, LGTM

wayneseymour · 2024-11-04T11:47:57Z

x-pack/plugins/security_solution/scripts/junit_transformer/lib.ts

+// Function to remove specific fields from an XML object in order to
+// compare them as strings.
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function removeFields(obj: any, fieldsToRemove: string[]): any {


Ok, I'd prefer an explicit test, but you sound confident enough in your response to me.
Thanks.

Oh, one last thing, I was not referring to coverage, but to my own personal nits about testing functions explicitly.
From your comment, it sounds like removeFields is only tested implicitly.
As this lib.ts file is not actually owned by my team, no worries.

MadameSheema · 2024-11-05T11:39:36Z

x-pack/plugins/security_solution/scripts/junit_transformer/lib.ts

+function removeFields(obj: any, fieldsToRemove: string[]): any {
+  for (const key in obj) {
+    if (fieldsToRemove.includes(key)) {
+      delete obj[key];


NIT: I believe it would be greate if we can avoid the modification of the original object by making a copy first and doing the modifications there with the aim of avoid side effects.

But we do not change the actual file at any point. We just getting a parsed string out of it and then we remove the fields for future use.

delanni

Prepend title 👍

kibanamachine · 2024-11-07T13:23:11Z

💚 All backports created successfully

Status	Branch	Result
✅	8.16
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…#197265) (#199313) # Backport This will backport the following commits from `main` to `8.16`: - [[Security Solution][Serverless] Github tickets / notifications (#197265)](#197265)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: dkirchan <55240027+dkirchan@users.noreply.github.com>

…ic#197265) ## Summary This PR is accompanied with the [PR in kibana-operations repo](elastic/kibana-operations#236) and integrates the security solution quality gate pipelines with the functionality to create ticket when a test fails and then notify the respective **codeowner** team with a _Test failed alert_. Also when a test fails a second time and a ticket exist a new comment is added in the same issue that the test failed again in a given pipeline. Last a similar flow exist when a test is skipped by a team. **Specifications:** - In the `failed_test_reporter_cli.ts` [a new field is introduced ](https://github.com/elastic/kibana/compare/main...security-mki-github-tickets#diff-bb801c18fd2e1a3a36a3b39fbf02c1abe337c46c201ad5a01239a9d2501d4b56R47)`prependTitle` which is initialized by the environmental variable `process.env.PREPEND_FAILURE_TITLE` if exists, in order to add a custom text of preference before the issue title. The scope of this is to be able to give the team the opportunity to add some tags or any convention agreed within the team in the issue created in order to easily understand the context without opening the ticket. If this field is not initialized, the normal existing flow proceeds. [Here](elastic#197133) an example of the prependTitle usage can be seen where the tags `[MKI][QA]` are prepended in order to identify where did the test fail, having the same exactly tests running on both CI and MKI. This means that a github issue with the exact same title would be created for both cases if this prepend title field would not exist. - In the [junit_transformer/lib.ts](elastic/kibana@main...security-mki-github-tickets#diff-31c5651af613c7d02139f3e9fccd00ddb997f2502523372dd19db9e0659a66d6R278) a new functionality is introduced to cover an existing issue. The existing issue is: the fact that we are retrying the failed test in cypress in the `parallel_serverless` script, leads to two junit xml result files completely identical with only difference the execution time and the timestamps. This change will take one by one the xml outputs, transform them exactly as it was happening before and then save the file, but also remove the time and timestamp fields, convert it to a string and add it to a "state" list. Then in a next file if it is already parsed and saved to the list having the exact same results within, it deletes the file instead of saving it transformed. The problem before this fix was the fact that when two xml outputs existed, a github ticket was created and when parsing-uploading the second file it was immediately failing for a second time as well. This means that `new-failure` notification was never triggered after the github actions flow split, and the existing failure was always triggered, something that most teams have disabled. With the fix, the identical files are parsed but only once uploaded and the new failure flow works again.

kibanamachine · 2024-11-11T13:48:59Z