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

feat!: json schemas for device config and mappings #837

Merged
merged 51 commits into from
Mar 21, 2023

Conversation

Julusian
Copy link
Member

@Julusian Julusian commented Jan 20, 2023

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

feature for R50

Depends on nrkno/sofie-timeline-state-resolver#237

  • What is the current behavior? (You can also link to an open issue here)

Gateways use a custom format for defining the config ui.
For TSR, this information is defined in playout-gateway, causing differences to occur between the manifest and the types in TSR.

  • What is the new behavior (if this is a feature change)?

Building on #809, this changes gateways to use a json-schema to define their configuration. It is expected that typescript interfaces should be generated from this schema to ensure that it stays in sync too.

  • Other information:

Support for the old ConfigManifest has been removed for the gateways, but the blueprint variant is still in use for blueprint based things currently. That should be expected to change in a future PR.

Ill admit that the SchemaForm* types are a little strange in how they are built on SchemaFormWithOverrides. After doing the initial step of device config with a basic SchemaForm type, I found that for Mappings which use the ObjectWithOverrides structure, it was necessary for a variation of the SchemaForm which supported this (including a variation of the table type). While implementing this I realised that a SchemaFormWithOverrides with no default values would behave the same as the basic SchemaForm type, so to avoid duplication and similar types which would drift apart in functionality over time I wrote the SchemaFormForCollection as a way of achieving this. SchemaFormForCollection turned out simpler than I hoped, so I am pretty happy with the result.

Status

  • Code documentation for the relevant parts in the code have been added/updated by the PR author
  • The functionality has been tested by the PR author
  • The functionality has been tested by NRK

@Julusian Julusian marked this pull request as ready for review January 23, 2023 18:02
@Julusian Julusian requested a review from a team January 23, 2023 18:03
@github-actions
Copy link

github-actions bot commented Jan 23, 2023

⚡ Published prerelease version shared-lib@1.49.0-nightly-feat-gateways-json-schema-20230307-151520-56499b3.0 to NPM

Copy link
Member

@jstarpl jstarpl left a comment

Choose a reason for hiding this comment

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

A couple of notes on feature parity with the Config Manifest system.

@jstarpl jstarpl requested a review from a team January 24, 2023 10:32
packages/scripts/schema-types.mjs Outdated Show resolved Hide resolved
@jstarpl
Copy link
Member

jstarpl commented Jan 25, 2023

One note I'd like to make on this PR is that it definitely breaks compatibility with any Peripheral Devices, so I would like to hold off on merging this in until we have a wider discussion on how to handle that, by for example bumping Core (and Playout Gateway, MOS Gateway, and Package Manager and Input Gateway) to 2.0.0

return (
<>
{' '}
{Object.entries(props.schema.properties || {}).map(([index, schema]) => {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this also take additionalProperties into consideration? I'm looking into how this affects Input Gateway, and have one instance where for one of the SubDevice types, I want to have a configuration of an object type, with user-configurable keys that then contains an object definition for that key that varies on a type discriminator property. In that case I used a ConfigManifestEntryType.TABLE with a defaultType, typeField and some config type definitions. I don't think this is possible here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont think additionalProperties is something the UI should be looking at. https://json-schema.org/understanding-json-schema/reference/object.html#additional-properties
The best we would be able to do if using it is allow for adding custom fields, using additionalProperties and patternProperties to limit the names/types of those. But they wont have a description or anything, so that wont be a good replication of what you were doing in input gateway.

I forgot to check input-gateway, so didn't realise that complex use of nested tables was being used. It could either be reimplemented using oneOf, or that table could be split up into multiple tables to not need the complex types

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I guess we can live with a multiple tables, I don't expect us to configure the MIDI integration all that often. Although I think we should leave some TODO here, since this is quite a significant limitation, I think.

@Julusian Julusian mentioned this pull request Mar 13, 2023
3 tasks
@jstarpl
Copy link
Member

jstarpl commented Mar 21, 2023

This is now ready to be merged into Release 50 branch.

@Julusian Julusian changed the base branch from release49 to release50 March 21, 2023 19:05
@Julusian Julusian merged commit fb8f1c3 into release50 Mar 21, 2023
@Julusian Julusian deleted the feat/gateways-json-schema branch March 21, 2023 19:16
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.

4 participants