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

[system] Adds tags.yml file so they appear under the Security Solution UI and upgrades package spec to version 3.0.0 #8206

Merged
merged 4 commits into from Oct 19, 2023

Conversation

marc-gr
Copy link
Contributor

@marc-gr marc-gr commented Oct 16, 2023

Proposed commit message

Adds tags.yml file so they appear under the Security Solution UI and upgrades package spec to version 3.0.0. Required changes include:

  • Removes dangling tags in dashboards.
  • Adds validation skip file for dashboard filters and search references.
  • Change env object type to flattened.
  • Remove yaml objects dotted notation.

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.

Related issues

@marc-gr marc-gr marked this pull request as ready for review October 16, 2023 08:47
@marc-gr marc-gr requested review from a team as code owners October 16, 2023 08:47
@elasticmachine
Copy link

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

@marc-gr marc-gr changed the title [system] Upgrade to package spec 3.0.0 [system] Adds tags.yml file so they appear under the Security Solution UI and upgrades package spec to version 3.0.0 Oct 16, 2023
@elasticmachine
Copy link

elasticmachine commented Oct 16, 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-10-18T06:03:25.964+0000

  • Duration: 16 min 12 sec

Test stats 🧪

Test Results
Failed 0
Passed 151
Skipped 0
Total 151

🤖 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

elasticmachine commented Oct 16, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (3/3) 💚
Files 100.0% (4/4) 💚 3.888
Classes 100.0% (4/4) 💚 3.888
Methods 63.415% (52/82) 👎 -27.939
Lines 99.863% (2924/2928) 👍 13.167
Conditionals 100.0% (0/0) 💚

@marc-gr
Copy link
Contributor Author

marc-gr commented Oct 16, 2023

/test

@pierrehilbert pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Oct 16, 2023
@elasticmachine
Copy link

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@marc-gr
Copy link
Contributor Author

marc-gr commented Oct 16, 2023

@jsoriano @mrodm It seems to me the errors we got loading test case failed: reading config for test case failed (testCasePath: /var/lib/jenkins/workspace/est-manager_integrations_PR-8206/src/github.com/elastic/integrations/packages/system/data_stream/auth/_dev/test/pipeline/test-auth-ubuntu1204.log): can't unpack test configuration: /var/lib/jenkins/workspace/est-manager_integrations_PR-8206/src/github.com/elastic/integrations/packages/system/data_stream/auth/_dev/test/pipeline/test-auth-ubuntu1204.log-config.yml: can not convert 'object' into 'string' accessing 'dynamic_fields.event' (source:'/var/lib/jenkins/workspace/est-manager_integrations_PR-8206/src/github.com/elastic/integrations/packages/system/data_stream/auth/_dev/test/pipeline/test-auth-ubuntu1204.log-config.yml') that got fixed in b606d36 should not fail in the first place? Is this intended?

@jsoriano
Copy link
Member

@marc-gr this is likely a side effect of elastic/elastic-package#1485. Try running elastic-package format on one of the affected packages, and you will see some changes in test files.

I am currently looking into making this automation a bit smarter. In the meantime you can workaround the issue by quoting the keys that the automation is converting to objects.

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

Great work!

@@ -2,6 +2,5 @@ fields:
event:
timezone: "+0000"
dynamic_fields:
event:
ingested: "^.*$"
event.ingested: "^.*$"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is funny. I take it that this is needed because it's essentially treated as a literal key rather than a path. Is that correct?

[a few moments later]

I see this is correct in the next commit.

@ishleenk17
Copy link
Contributor

@marc-gr : The upgrade of system package to 3.0.0 is being done as part of this effort.
I am not sure if these two are overlap ?

@ishleenk17
Copy link
Contributor

@marc-gr : The upgrade of system package to 3.0.0 is being done as part of this effort. I am not sure if these two are overlap ?

Checked with infraobs team. Looks like this has some security related changes as well.
So we can take this change for system package

Comment on lines +2 to +3
event:
timezone: "+0000"
Copy link
Member

@shmsr shmsr Oct 19, 2023

Choose a reason for hiding this comment

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

Suggested change
event:
timezone: "+0000"
event:
timezone: "+0000"

@jsoriano If I am not wrong this also needs to be quoted ("event.timezone"), right?

PR with similar changes: #8174

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to only fail for dynamic_fields

Copy link
Member

Choose a reason for hiding this comment

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

We have reverted the change in elastic-package that was requiring all these changes in test config files. So you can also revert all these changes, they should not be needed anymore.

@marc-gr marc-gr merged commit 22b5c72 into elastic:main Oct 19, 2023
4 checks passed
@marc-gr marc-gr deleted the system-package-spec-3 branch October 19, 2023 11:50
@elasticmachine
Copy link

Package system - 1.45.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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants