Skip to content

System package: revert back change to dynamic_dataset/namespace#6801

Merged
gsantoro merged 3 commits intoelastic:mainfrom
gsantoro:feature/system_dynamic_dataset
Jul 7, 2023
Merged

System package: revert back change to dynamic_dataset/namespace#6801
gsantoro merged 3 commits intoelastic:mainfrom
gsantoro:feature/system_dynamic_dataset

Conversation

@gsantoro
Copy link
Copy Markdown
Contributor

@gsantoro gsantoro commented Jul 4, 2023 •
edited
Loading

What does this PR do?

Revert back changes from issue elastic/kibana#157897.

Removing the following configs from the system package manifest

elasticsearch.dynamic_dataset: true
elasticsearch.dynamic_namespace: true

We would like to revert these permissions in favour of creating a separate input package to reroute traffic from Syslog.

Also, syslog datastream is enabled by default when you create a new agent policy. By having these permissions here, we were accidentally giving any elastic-agent permissions to write logs-*-*.

The idea is that we would like to have datastream-based integrations to monitor a specific application, while we should have input packages instead to reroute "mixed" traffic (from different sources) from that application.

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

@gsantoro gsantoro added the enhancement New feature or request label Jul 4, 2023
@gsantoro gsantoro requested a review from a team as a code owner July 4, 2023 09:53
@gsantoro gsantoro requested review from felixbarny and ruflin July 4, 2023 09:54
@gsantoro gsantoro added the v8.9.0 label Jul 4, 2023
@gsantoro gsantoro self-assigned this Jul 4, 2023
@elasticmachine
Copy link
Copy Markdown

elasticmachine commented Jul 4, 2023 •
edited
Loading

💚 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-07-06T09:52:35.809+0000

  • Duration: 15 min 47 sec

Test stats 🧪

Test Results
Failed 0
Passed 146
Skipped 0
Total 146

🤖 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
Copy Markdown

elasticmachine commented Jul 4, 2023 •
edited
Loading

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (3/3) 💚
Files 100.0% (4/4) 💚
Classes 100.0% (4/4) 💚
Methods 60.759% (48/79) 👎 -27.812
Lines 100.0% (2811/2811) 💚 7.532
Conditionals 100.0% (0/0) 💚

ruflin
ruflin approved these changes Jul 4, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM

@ishleenk17
Copy link
Copy Markdown
Member

ishleenk17 commented Jul 4, 2023

@gsantoro : Are we not adding re-route processor to the system package?
What is the reason for this revert?

@gsantoro
Copy link
Copy Markdown
Contributor Author

gsantoro commented Jul 5, 2023

hello @ishleenk17,
I have now updated the description. Let me know if it makes sense.

ishleenk17
ishleenk17 reviewed Jul 6, 2023
View reviewed changes
packages/system/data_stream/network/fields/agent.yml Outdated
example: "stretch"
description: >
OS codename, if any.

Copy link
Copy Markdown
Member

@ishleenk17 ishleenk17 Jul 6, 2023

Choose a reason for hiding this comment

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

Please remove this

Copy link
Copy Markdown
Contributor Author

@gsantoro gsantoro Jul 6, 2023

Choose a reason for hiding this comment

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

I'm a bit conflicted by this change since even if I agree that it is not relevant to this PR, it was introduced by running elastic-package format locally.

Every time I run this command locally to test the integration I have to revert those formatting changes manually. This time, I committed this change by "mistake".

While this package has only this file change, some packages are not well formatted in multiple files.

Is it just me or do other people have similar issues?

@ishleenk17
Copy link
Copy Markdown
Member

ishleenk17 commented Jul 6, 2023

hello @ishleenk17, I have now updated the description. Let me know if it makes sense.

Does this mean we are going to create Input package for this Integration that would collect traffic in one place from a common input type for System Package?

ishleenk17
ishleenk17 approved these changes Jul 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ishleenk17 ishleenk17 left a comment

Choose a reason for hiding this comment

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

Looks good

@gsantoro gsantoro merged commit 91b3060 into elastic:main Jul 7, 2023
@gsantoro gsantoro deleted the feature/system_dynamic_dataset branch July 7, 2023 09:19
@elasticmachine
Copy link
Copy Markdown

elasticmachine commented Jul 7, 2023

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

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

Reviewers

@ruflin ruflin ruflin approved these changes

@ishleenk17 ishleenk17 ishleenk17 approved these changes

@felixbarny felixbarny Awaiting requested review from felixbarny

Assignees

@gsantoro gsantoro

Labels

enhancement New feature or request Integration:system System v8.9.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gsantoro @elasticmachine @ishleenk17 @ruflin @andrewkroh @gbamparop