Migrate obs-infraobs-integrations to package-spec v3 #6 #8291
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
| @@ -23,3 +23,4 @@ | |||
| - name: metrics.* | |||
| description: Default mapping for metric fields. You need to add your own custom mappings when storing other kinds of information. | |||
| type: object | |||
| object_type: keyword | |||
There was a problem hiding this comment.
this is a weird one - we don't want to put a type here at all, we want to keep the mapping dynamic for all subfields in metrics which would be something like
name: metrics
type: object
dynamic: true
the only problem with this is that we get failing validations here if we include any metrics.* fields in sample_event.json.
removing the values from the sample event doesn't seem right, because the mapping supports them. i wonder what the right course of action is here?
There was a problem hiding this comment.
Isn't object_type a must now, as per elastic/package-spec#628 ?
There was a problem hiding this comment.
the intention there is to remove ambiguous mappings, but in this case we want the mapping to be ambiguous.
there's no sensible value we can set here for object_type.
There was a problem hiding this comment.
This will be true for most Input Packages, I suppose. Since the mapping has to be dynamic.
The metrics.* was added to have some mapping for the system tests to run to have the sample event generated
There was a problem hiding this comment.
i guess we have two options here then - we can either remove the metrics.* mapping entirely, and ensure we document that we expect users to define custom mappings, or we use some sensible default numerical value, like double.
i'm happy with either to get us unblocked. what do you all think?
There was a problem hiding this comment.
The windows package also has this same challenge, and it's preventing an upgrade to a newer spec. Any updates on how to handle this blocker?
There was a problem hiding this comment.
I started working on the windows package before I noticed Eric's comments about it and of course ran into the same issue. I'm also running into this same mapping issue with https://github.com/elastic/security-team/issues/7388 (moving to package-spec 3.0.2 includes this validation and a number of integrations are failing said validation).
Echoing Eric's comment above, any updates/ideas on how to handle this blocker? We can't remove the mappings, since our example data includes these fields, and in at least one integration, the sub-fields for a field span multiple types, so object_type isn't going to work there.
There was a problem hiding this comment.
Apologies, @ebeahan, as I seem to have missed your comment. As for you, @taylor-swanson, we have not yet reached a conclusion, which is why the PR is still open. Although we did discuss it in our team meeting once (points that @tommyers-elastic suggested above), we were unable to come to a decision at that time. I will make an effort to communicate with my team again regarding this matter since the PR is still stuck on it.
There was a problem hiding this comment.
For this case, if the problem is that the test is generating fields under this untyped object, maybe we could try to configure the test so it uses the typed fields. Or if we want to explicitly test these untyped fields, maybe we need to add support to elastic-package test to include custom mappings.
In general, if these objects are included only for testing and documentation purposes, and it is expected for users to provide custom mappings if they want to use these fields, something you could try is to use index: false. This is also an option if you want these fields to be in the source just in case, but they are not expected to be directly used.
In principle this would be allowed by the spec, but these subfields won't be indexed unless some custom mapping is provided.
- name: metrics.*
description: Default mapping for metric fields. You need to add your own custom mappings when storing other kinds of information.
type: object
index: false
|
Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as |
|
Hi @shmsr, please update your branch with the latest contents from main branch. There was an important PR merged updating the CI pipelines. Thanks! |
|
Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as |
|
Hi! This PR has been stale for a while and we're going to close it as part of our cleanup procedure. We appreciate your contribution and would like to apologize if we have not been able to review it, due to the current heavy load of the team. Feel free to re-open this PR if you think it should stay open and is worth rebasing. Thank you for your contribution! |
Follow discussion (from the PR these packages are pulled from as they were blocking changes getting merged for other packages):
Proposed commit message
Pulled out the following packages from #8203 as it is blocking package-spec v3 migration for other packages in that PR. As these packages have some confusion in their mappings, pulled out the changes to this branch.
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally
Related issues
Screenshots