Skip to content

Update dependency on elastic-package #987

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
Merged
merged 7 commits into from
May 13, 2021
Merged

Update dependency on elastic-package #987

merged 7 commits into from
May 13, 2021

Conversation

mtojek
Copy link
Contributor

@mtojek mtojek commented May 13, 2021

This PR bumps up dependency on the elastic-package (changes around semantic validation of spec).

@mtojek mtojek self-assigned this May 13, 2021
@mtojek
Copy link
Contributor Author

mtojek commented May 13, 2021 •
edited
Loading

@ycombinator @rw-access I can't figure out which format of security_rule is correct:

In spec:
https://github.com/elastic/package-spec/blob/master/test/packages/good/kibana/security_rule/000047bb-b27a-47ec-8b62-ef1a5d2c9e19.json

In package:
https://github.com/elastic/integrations/blob/master/packages/security_detection_engine/kibana/security_rule/rule-000047bb-b27a-47ec-8b62-ef1a5d2c9e19.json

Please look at the "rule_id" and "id".

@elasticmachine
Copy link

elasticmachine commented May 13, 2021 •
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

Expand to view the summary

Build stats

  • Build Cause: Pull request #987 updated

  • Start Time: 2021-05-13T19:45:41.145+0000

  • Duration: 32 min 32 sec

  • Commit: 4914160

Test stats 🧪

Test Results
Failed 0
Passed 2132
Skipped 3
Total 2135

Trends 🧪

Image of Build Times

Image of Tests

@mtojek mtojek mentioned this pull request May 13, 2021
Closed
@rw-access
Copy link
Contributor

Oh! I didn't realize spec was validating anything other than JSON so the spec was out of date. The format inside the integration that's structured like a SO is correct.

@mtojek
Copy link
Contributor Author

mtojek commented May 13, 2021

Thanks @rw-access for clarifying this. It seems that we need to fix the package-spec.

@mtojek mtojek mentioned this pull request May 13, 2021
Merged
2 tasks
@mtojek mtojek requested a review from ycombinator May 13, 2021 19:11
ycombinator
ycombinator reviewed May 13, 2021
View reviewed changes
packages/mongodb/kibana/visualization/mongodb-operation-counters.json
@@ -175,7 +175,7 @@
],
"references": [
{
"id": "mongodb-MongoDB-search",
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes seem unrelated to the security rule naming changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linting failed. It seems that the spec hasn't been updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. You're referring to the commit name.

rw-access
rw-access previously requested changes May 13, 2021
View reviewed changes
...ecurity_detection_engine/kibana/security_rule/rule-000047bb-b27a-47ec-8b62-ef1a5d2c9e19.json Outdated
@@ -22,7 +22,7 @@
"https://developer.okta.com/docs/reference/api/event-types/"
],
"risk_score": 21,
"rule_id": "000047bb-b27a-47ec-8b62-ef1a5d2c9e19",
"rule_id": "rule-000047bb-b27a-47ec-8b62-ef1a5d2c9e19",
Copy link
Contributor

Choose a reason for hiding this comment

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

wait this isn't right. I don't understand why this happened

Copy link
Contributor Author

@mtojek mtojek May 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We have a rule that ID should consistent with filename. Do you prefer to go the other way round, I mean to rename the file?

Copy link
Contributor

Choose a reason for hiding this comment

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

The rule ID can't change, it's defined elsewhere. So we could change the filename. That's fine with me.

I don't really understand why it's a hard and fast requirement or why the change is necessary, to be honest.

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's for readability and clarity purposes. We encountered misnaming in the past, e.g. "access-logs-dashboard.json" which contains "error-logs-overview". This way we simplify navigating through resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

...ecurity_detection_engine/kibana/security_rule/rule-000047bb-b27a-47ec-8b62-ef1a5d2c9e19.json Outdated
@@ -36,6 +36,6 @@
"type": "query",
"version": 5
},
"id": "000047bb-b27a-47ec-8b62-ef1a5d2c9e19",
"id": "rule-000047bb-b27a-47ec-8b62-ef1a5d2c9e19",
Copy link
Contributor

Choose a reason for hiding this comment

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

Or this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@mtojek mtojek requested review from rw-access and ycombinator May 13, 2021 19:45
rw-access
rw-access approved these changes May 13, 2021
View reviewed changes
Copy link
Contributor

@rw-access rw-access left a comment

Choose a reason for hiding this comment

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

LGTM. thanks for your patience @mtojek

ycombinator
ycombinator approved these changes May 13, 2021
View reviewed changes
Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM.

@mtojek mtojek merged commit e1c09f7 into elastic:master May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ycombinator ycombinator ycombinator approved these changes

@rw-access rw-access rw-access approved these changes

Assignees

@mtojek mtojek

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mtojek @elasticmachine @rw-access @ycombinator