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.

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

cisco_asa,cisco_ftd,microsoft_defender_endpoint,proofpoint_tap,slack: ensure event.type holds ECS-compliant values #7926

Closed
wants to merge 1 commit into from

Conversation

efd6
Copy link
Contributor

@efd6 efd6 commented Sep 22, 2023

What does this PR do?

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

@efd6 efd6 self-assigned this Sep 22, 2023
@efd6 efd6 changed the title cisco_asa,cisco_ftd,microsofl_defender_endpoint,proofpoint_tap,slack: ensure event.type holds ECS-compliant values cisco_asa,cisco_ftd,microsoft_defender_endpoint,proofpoint_tap,slack: ensure event.type holds ECS-compliant values Sep 22, 2023
… ensure event.type holds ECS-compliant values
@elasticmachine
Copy link

elasticmachine commented Sep 22, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-09-22T00:54:04.564+0000

  • Duration: 17 min 14 sec

Test stats 🧪

Test Results
Failed 0
Passed 94
Skipped 0
Total 94

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (9/9) 💚
Files 100.0% (55/55) 💚 1.481
Classes 100.0% (55/55) 💚 1.481
Methods 99.298% (283/285) 👍 1.65
Lines 85.41% (7909/9260) 👎 -8.209
Conditionals 100.0% (0/0) 💚

@efd6 efd6 marked this pull request as ready for review September 22, 2023 01:15
@efd6 efd6 requested a review from a team as a code owner September 22, 2023 01:15
@elasticmachine
Copy link

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@efd6
Copy link
Contributor Author

efd6 commented Sep 22, 2023

What has me confused here is how we ended up with these in this state in the repo, unless it is because EP now tests this and previously (when they were last touched) did not. @jsoriano Does bumping the EP version not cause all packages to be tested? Oh, it seems it does, but failures are tolerated and no notification is sent to the package owners.

@jsoriano
Copy link
Member

What has me confused here is how we ended up with these in this state in the repo

Hey @efd6, sorry for the confusion.

There was a regression in field values validation introduced in elastic-package 0.84.0, see elastic/elastic-package#1439.

When fixing it in elastic-package 0.87.1, we decided to force merge the update in the integrations repository, even if it was failing for a number of packages, to avoid introducing the issue in more packages. See these comments elastic/elastic-package#1439 (comment)

@@ -1,4 +1,9 @@
# newer versions go on top
- version: "2.22.1"
changes:
- description: Ensure `event.type` is not set to ECS-noncompliant values.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- description: Ensure `event.type` is not set to ECS-noncompliant values.
- description: Ensure `event.type` is set to ECS-compliant values.

🤷 wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about that, to avoid the double negative, but they're semantically non-overlapping. It looks like the point is moot, since I think this was fixed in work by @kgeller.

@kgeller
Copy link
Contributor

kgeller commented Sep 22, 2023

Hey @efd6 I've been working through fixing these as I go through the ECS 8.10 updates, since they all cause the update tool to fail.

I merged PRs for cisco_asa and cisco_ftd already, and have several of the others up that are linked to the ECS update issue and the elastic-package issue. I think by the time I finish, it should encompass what you've got here.

@efd6 efd6 closed this Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Integration:cisco_asa Cisco ASA Integration:cisco_ftd Cisco FTD Integration:microsoft_defender_endpoint Microsoft Defender for Endpoint Integration:proofpoint_tap Proofpoint TAP Integration:slack Slack Logs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants