-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Usage Collection] [schema] Explicit "array" definition #78141
[Usage Collection] [schema] Explicit "array" definition #78141
Conversation
0327ccc
to
39a8515
Compare
Pinging @elastic/kibana-telemetry (Team:KibanaTelemetry) |
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.
Code review only -- Security plugin changes LGTM!
Pinging @elastic/uptime (Team:uptime) |
Pinging @elastic/ingest-management (Team:Ingest Management) |
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.
Ingest Management changes look good, thank you! 👍
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.
Security Solution, changes look 💯. Thank you!
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.
Thank you for adding the array type! It will make it much easier to interpret what share the data is in.
LGTM
@@ -44,7 +47,10 @@ | |||
"type": "keyword" | |||
}, | |||
"uninstalled": { | |||
"type": "keyword" | |||
"type": "array", | |||
"items": { |
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.
What do you think about renaming items
to properties
?
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.
I tried to follow similarities to the JSON-schema specification (even though we are not fully using that).
I think items
or an array make more sense to me. They are not properties
or the array.
But happy to change it if you think properties
is the right way to go.
@@ -28,7 +28,7 @@ export type AllowedSchemaTypes = | |||
| 'date' | |||
| 'float'; | |||
|
|||
export function compatibleSchemaTypes(type: AllowedSchemaTypes) { | |||
export function compatibleSchemaTypes(type: AllowedSchemaTypes | 'array') { |
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.
Why didnt we add array
to the AllowedSchemaTypes
type here instead of using union?
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.
Because I think TS would allow { type: 'array' }
for non-array fields and we'll only catch it when running the check.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
fd9d9bf
to
6c6e4b3
Compare
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.
LGTM Uptime changes !!
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
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.
LGTM! Thanks!
#78618) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: Fix APM lodash imports (elastic#78438) Add deprecated message to tile_map and region_map visualizations. (elastic#77683) Fix Lens smokescreen flaky tests (elastic#78566) updated discover with alt text (elastic#77660) Fix types (elastic#78619) Update tutorial-visualizing.asciidoc (elastic#76977) Update tutorial-discovering.asciidoc (elastic#76976) [Search] Error notification alignment (elastic#77788) Update tutorial-define-index.asciidoc (elastic#76975) [Lens] Fieldless operations (elastic#78080) [Usage Collection] [schema] Explicit "array" definition (elastic#78141) Update tutorial-define-index.asciidoc (elastic#76973) Fix --no-basepath references in doc (elastic#78570) Move StubIndexPattern to data plugin and convert to TS. (elastic#78518) Index pattern class - remove unused methods (elastic#78538) [Security Solution] [ALL] Eliminates all console.error and console.warn from Jest output (elastic#78523) [Actions] avoids setting a default dedupKey on PagerDuty (elastic#77773) First stab at developer-focussed saved objects docs (elastic#71430) remove unnecessary config validations (elastic#78527)
Pinging @elastic/kibana-core (Team:Core) |
Summary
We want the usage collection schema to be declarative of the format that is reported by each registered usage collector. That's why we are adding a specific way to declare array properties.
Checklist
Delete any items that are not applicable to this PR.
For maintainers