-
Notifications
You must be signed in to change notification settings - Fork 77
Add support to discover content packages based on datasets #1338
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
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
2f4de8f to
f676e40
Compare
1554fd7 to
dc6fed9
Compare
| } | ||
|
|
||
| if v := query.Get("discovery"); v != "" { | ||
| for _, v := range query["discovery"] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change required to allow get the values of all the discovery query parameter if it is set several times.
| func (f *discoveryFilter) Matches(p *Package) bool { | ||
| if f == nil { | ||
| return true | ||
| } | ||
| return f.Fields.Matches(p) | ||
| if len(f.Fields) > 0 && !f.Fields.Matches(p) { | ||
| return false | ||
| } | ||
| if len(f.Datasets) > 0 && !f.Datasets.Matches(p) { | ||
| return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way it has been implemented, in order to add a package into a response, the filter must match the given package for fields and for datasets.
So in a query like
GET /search?discovery=fields:process.pid&discovery=datasets:nginx.error
The packages added to the response must fulfill these two conditions:
- define just
process.idin theirdiscovery.fields, - define at least
nginx.errorin theirdiscovery.datasets.
| "categories": [ | ||
| "support" | ||
| ], | ||
| "discovery": {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This key is shown because discovery is not empty:
- Package manifest:
discovery:
fields: []
datasets: []- Field definition in
BasePackagestruct:
Discovery *Discovery `config:"discovery,omitempty" json:"discovery,omitempty" yaml:"discovery,omitempty"`There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would omitzero help here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks it could be used to allow not show discovery: {} in the response.
I'll push a change to include this tag for json.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed change in f6825ec
| "categories": [ | ||
| "support" | ||
| ], | ||
| "discovery": {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would omitzero help here?
Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
💚 Build Succeeded
History
cc @mrodm |
Closes #1336
This PR adds support to discover packages based on the datasets defined in the discovery parameter:
Example of query:
If there are multiple discovery parameters in the query, like:
packages must match all filters at the same time. In this example:
process.pidas its only discovery field.nginx.errorin their discovery datasets.Author's checklist
discovery.datasetsfield.discoveryquery parameter.discovery.fieldsfilter.How to test this PR locally