Skip to content

Update dependency on elastic-package#987

Merged
mtojek merged 7 commits intoelastic:masterfrom
mtojek:update-ep-1305
May 13, 2021
Merged

Update dependency on elastic-package#987
mtojek merged 7 commits intoelastic:masterfrom
mtojek:update-ep-1305

Conversation

@mtojek
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

rw-access commented May 13, 2021

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
Copy Markdown
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 Outdated
Copy link
Copy Markdown
Contributor

@ycombinator ycombinator May 13, 2021

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
Copy Markdown
Contributor Author

@mtojek mtojek May 13, 2021

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
Copy Markdown
Contributor Author

@mtojek mtojek May 13, 2021

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
Copy link
Copy Markdown
Contributor

@rw-access rw-access May 13, 2021

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

@rw-access rw-access May 13, 2021

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
Copy Markdown
Contributor Author

@mtojek mtojek May 13, 2021

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
Copy Markdown
Contributor Author

@mtojek mtojek May 13, 2021

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
Copy link
Copy Markdown
Contributor

@rw-access rw-access May 13, 2021

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
Copy Markdown
Contributor Author

@mtojek mtojek May 13, 2021

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

+1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

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