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

ti_maltiverse: move non-ECS fields out of root #7909

Merged
merged 2 commits into from Sep 26, 2023
Merged

ti_maltiverse: move non-ECS fields out of root #7909

merged 2 commits into from Sep 26, 2023

Conversation

efd6
Copy link
Contributor

@efd6 efd6 commented Sep 21, 2023

What does this PR do?

See title.

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 21, 2023
@elasticmachine
Copy link

elasticmachine commented Sep 21, 2023 •
edited

💚 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-24T23:18:49.108+0000

  • Duration: 15 min 21 sec

Test stats 🧪

Test Results
Failed 0
Passed 5
Skipped 0
Total 5

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

@efd6 efd6 force-pushed the 7874-ti_maltiverse branch 2 times, most recently from 9ccdbfa to 9dcf850 Compare September 21, 2023 06:31
@elasticmachine
Copy link

elasticmachine commented Sep 21, 2023 •
edited

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (1/1) 💚
Files 100.0% (1/1) 💚
Classes 100.0% (1/1) 💚
Methods 92.308% (12/13) 👎 -7.692
Lines 75.564% (201/266) 👎 -22.597
Conditionals 100.0% (0/0) 💚

@P1llus
Copy link
Member

P1llus commented Sep 21, 2023

The system tests failing for undefined fields was related to the fields.yml for the transform not being updated.
The system test outputs the incorrect related index name which causes confusion. Added a tracking issue for that here: elastic/elastic-package#1463

System tests for transforms was added here: https://github.com/elastic/elastic-package/pull/1409/files

efd6
efd6 commented Sep 21, 2023
View reviewed changes
packages/ti_maltiverse/data_stream/indicator/fields/fields.yml
type: long
description: number of reports for the indicator
- name: external_references
type: flattened
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This must be flattened because the transform imposes an index sort order and this is not compatible with nested.

@efd6 efd6 marked this pull request as ready for review September 21, 2023 23:20
@efd6 efd6 requested a review from a team as a code owner September 21, 2023 23:20
@elasticmachine
Copy link

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

andrewkroh
andrewkroh reviewed Sep 22, 2023
View reviewed changes
packages/ti_maltiverse/data_stream/indicator/fields/fields.yml Outdated
type: boolean
description: boolean description tag
- name: location
type: boolean
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this field should be a geo_point and maltiverse.location.{lat,lon} should be removed.

The same applies to the transform fields.

packages/ti_maltiverse/data_stream/indicator/fields/fields.yml Outdated
type: text
description: CIDR associated
- name: city
type: text
Copy link
Member

Choose a reason for hiding this comment

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

A lot these text fields should be keyword in my opinion. If they need to be searchable then we could add a multi-field as needed for text or match_only_text.

@efd6 efd6 merged commit e70d580 into elastic:main Sep 26, 2023
4 checks passed
@elasticmachine
Copy link

Package ti_maltiverse - 0.4.0 containing this change is available at https://epr.elastic.co/search?package=ti_maltiverse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewkroh andrewkroh andrewkroh approved these changes

Assignees

@efd6 efd6

Labels
bug Something isn't working Integration:Maltiverse TI Maltiverse
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ti_maltiverse] Must namespace non-ECS fields
4 participants
@efd6 @elasticmachine @P1llus @andrewkroh