Skip to content
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

Update mapper output schema to allow multiple devices per message #496

Merged
merged 5 commits into from
Dec 7, 2022

Conversation

gnmerritt
Copy link
Contributor

This adds the additional required fields to the mapper output schema to allow for multiple translations to be issued at once (via a map of guid -> entity info) and adds an extra level to stash entity information like type, missing_telemetry_fields, and connections.

schema/event_mapping_entities.json Outdated Show resolved Hide resolved
schema/event_mapping_entity.json Show resolved Hide resolved
schema/event_mapping_entity.json Outdated Show resolved Hide resolved
schema/event_mapping_entity.json Show resolved Hide resolved
schema/event_mapping_entity.json Show resolved Hide resolved
schema/event_mapping_entity.json Show resolved Hide resolved
schema/event_mapping_entity.json Outdated Show resolved Hide resolved
schema/event_mapping_entity.json Outdated Show resolved Hide resolved
@grafnu
Copy link
Collaborator

grafnu commented Nov 4, 2022 via email

@bsimmons-onboard
Copy link

@grafnu we did a second pass based on your feedback, ready for another look.

schema/event_mapping_entities.json Outdated Show resolved Hide resolved
schema/event_mapping_entity.json Show resolved Hide resolved
schema/event_mapping_entity.json Show resolved Hide resolved
schema/event_mapping_entity.json Outdated Show resolved Hide resolved
schema/event_mapping_entity.json Show resolved Hide resolved
@gnmerritt
Copy link
Contributor Author

Okay - added patterns in all the spots you requested, thank you for the suggestions @grafnu. The only one that I didn't use was code - I see a bunch of mixed case values in our examples and it's a human readable field so I think we should be fine on comparisons.

I rebased and rebuilt the gencode hash but otherwise there are no changes in the earlier force-pushed commits

Copy link
Collaborator

@grafnu grafnu left a comment

Choose a reason for hiding this comment

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

Still two lingering bits that are just "type: object" -- which should be made more specific...

schema/virtual_links.json Show resolved Hide resolved
schema/event_mapping_entity.json Show resolved Hide resolved
@grafnu
Copy link
Collaborator

grafnu commented Dec 1, 2022 via email

Copy link
Contributor Author

@gnmerritt gnmerritt left a comment

Choose a reason for hiding this comment

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

Hi Trevor - I tried to flag my changes explicitly in this review, hopefully that will make it easier for you/GitHub's UI

edit: doesn't seem to have helped much, so I'll clarify below:

In a couple spots I am proposing a schema which includes map objects with un-enumerable string keys (guids, DBO types, etc). I believe the correct expression of such a thing in JSON-schema-speak is "type": "object" with additional validation of the key pattern and value shape provided in the "patternProperties" field.

You seem to not want any "type": "object" fields. If there is another way you'd like me to try and express these same relationships please provide me with an example that would be acceptable to you, as I don't seem to be making much progress guessing at your intent.

schema/event_mapping_entity.json Show resolved Hide resolved
schema/virtual_links.json Show resolved Hide resolved
Copy link
Collaborator

@grafnu grafnu left a comment

Choose a reason for hiding this comment

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

Looks good! I'm sure there will be iterations at some point, but nothing is jumping out me as wrong at this point...

@gnmerritt
Copy link
Contributor Author

@grafnu I don't have write permissions on the repo - is there anything else that needs to happen before this merges?

@grafnu grafnu merged commit 40806c6 into faucetsdn:master Dec 7, 2022
@grafnu
Copy link
Collaborator

grafnu commented Dec 7, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants