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

Add Sources to API #18

Merged
merged 9 commits into from
Feb 21, 2024
Merged

Add Sources to API #18

merged 9 commits into from
Feb 21, 2024

Conversation

samdbmg
Copy link
Member

@samdbmg samdbmg commented Jan 31, 2024

Details

  • Adds an ADR document describing adding Sources as richer objects on the API
  • Adds proposed implementation

Pivotal Story (if relevant)

Story URL: https://www.pivotaltracker.com/story/show/186711160

Related PRs

Followed by #19

Submitter PR Checks

(tick as appropriate)

  • Added bbc/rd-apmm-cloudfit team as a reviewer
  • PR completes task/fixes bug
  • API version has been incremented if necessary
  • ADR status has been updated, and ADR implementation has been recorded
  • Documentation updated (README, etc.)
  • PR added to Pivotal story (if relevant)
  • Follow-up stories added to Pivotal

Reviewer PR Checks

(tick as appropriate)

  • PR completes task/fixes bug
  • Design makes sense, and fits with our current code base
  • Code is easy to follow
  • PR size is sensible
  • Commit history is sensible and tidy

Info on PRs

The checks above are guidelines. They don't all have to be ticked, but they should all have been considered.

Adds a missing schema reference for the Flow schema so that it appears
in rendered documentation
@samdbmg samdbmg force-pushed the sammg-add-sources-to-api branch from e4f6ebf to 5a81a5d Compare January 31, 2024 15:59
@samdbmg samdbmg marked this pull request as ready for review January 31, 2024 16:01
@samdbmg samdbmg requested a review from a team January 31, 2024 16:01
@philipnbbc philipnbbc self-assigned this Feb 1, 2024
philipnbbc
philipnbbc previously approved these changes Feb 1, 2024
Copy link
Contributor

@philipnbbc philipnbbc left a comment

Choose a reason for hiding this comment

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

Minor thought about using Flow IDs not being at odds with the model, but otherwise LGTM

docs/adr/decisions/0002-add-sources-to-api.md Outdated Show resolved Hide resolved
docs/adr/decisions/0002-add-sources-to-api.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@j616
Copy link
Contributor

j616 commented Feb 8, 2024

This implementation diverges from NMOS in some ways such as the exclusion of the high level media type (e.g. audio/video/mux/data) from the Source metadata. I'd suggest that unless we have a specific reason to diverge, we should build on previous work and include that metadata.

api/schemas/source.json Outdated Show resolved Hide resolved
@samdbmg samdbmg force-pushed the sammg-add-sources-to-api branch from a638608 to 0800e3a Compare February 16, 2024 12:43
@samdbmg
Copy link
Member Author

samdbmg commented Feb 16, 2024

ADR document updated with discussion outcomes from Tuesday's session - I'll request re-review before merge

@samdbmg samdbmg dismissed philipnbbc’s stale review February 16, 2024 12:43

Substantial changes made following discussion

Copy link
Contributor

@philipnbbc philipnbbc left a comment

Choose a reason for hiding this comment

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

A suggestion to remove the (manual) API version change and otherwise LGTM.

api/TimeAddressableMediaStore.yaml Outdated Show resolved Hide resolved
Adds details of workshop discussions to Sources ADR, along with a new
Option 1a for the conclusion of that discussion: that it should be
accepted without the Source graph/hierarchy piece.
Removes the `contains` and `contributes_to` properties of the Sources
proposal (for ADR-0002) pending further consideration, that were added
by c406e99.
Makes corresponding change to documentation.
@samdbmg samdbmg force-pushed the sammg-add-sources-to-api branch from cf23998 to 3128339 Compare February 21, 2024 09:49
@samdbmg samdbmg merged commit 9582add into main Feb 21, 2024
3 checks passed
@samdbmg samdbmg deleted the sammg-add-sources-to-api branch February 21, 2024 09:51
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