-
Notifications
You must be signed in to change notification settings - Fork 7
Flow profiles #130
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
base: main
Are you sure you want to change the base?
Flow profiles #130
Conversation
himslm01
left a comment
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.
A few readability improvements.
Updated wording Co-authored-by: James Sandford <james.sandford@bbc.co.uk>
… flow-profiles
… flow-profiles
samdbmg
left a comment
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.
Looks good to me overall: a good approach of maintaining flexibility while constraining in a way to make it easier to use.
I've inlined a handful of suggestions, and a couple of nits
|
I've also kicked off the CI for you, so you should be able to see if it passes validation |
api/schemas/flow-technical.json
Outdated
| { | ||
| "type": "object", | ||
| "description": "Describes the technical characteristics of a Flow (common properties to all Flows, imported by type-specific specifications)", | ||
| "title": "Flow Core", |
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.
NIT: Would be good to have the title updated as it keeps the rendered version more readable, there are currently two schemas rendered out as Flow Core
Co-authored-by: Sam Mesterton-Gibbons <sam@samn.co.uk>
Co-authored-by: Sam Mesterton-Gibbons <sam@samn.co.uk>
… flow-profiles
Co-authored-by: Georgina Shippey <9370167+GeorginaShippey@users.noreply.github.com>
Co-authored-by: Georgina Shippey <9370167+GeorginaShippey@users.noreply.github.com>
… flow-profiles
danjhd
left a comment
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.
Also this may be a nit and BBC R&D can guide but my feeling is that when referring to TAMS entity types like Flow(s). Source(s) or Profile(s) we should always capitalise these words to help clarify?
|
|
||
| When using the second option on submission of the create flow request, the store will then read the metadata for a given profile and use this to populate the metadata for the flow. | ||
| This de-normalisation of the technical parameters means that on the read side the flows created via both mechanisms have the same technical parameters available. | ||
| Additionally if the decision is made to change a standard profile in the future then this does not affect the existing flows as the parameters have already been replicated and should remain unchanged to reflect the media actually being stored. |
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.
Above you mention, quite rightly i believe, that Flows should be immutable. Yet here you talk about a use case where a standard profile could be changed. This conflicts. I don't believe we should allow this use case. As you say above if the profile needs to change a new one with a new Id should be created and used.
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.
Good point - we should remove this as not required. Will re-work this section anyway based on another comment
| 1. Specify all the technical characteristics of the flow including the wrapper, codec and essence parameters alongside the non-technical parameters such as label and description | ||
| 2. Provide just the technical profile for the flow and non-technical parameters required | ||
|
|
||
| When using the second option on submission of the create flow request, the store will then read the metadata for a given profile and use this to populate the metadata for the flow. |
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.
It feels here like we are making a decision for the implementation? As long as the GET request to the Flow returns all the metadata it should not matter how the implementation does this. i.e. it might not populate them it might just keep the reference to the profile id and "hydrate" on request? The important point is that when GETting Flows a client should be able to see all the metadata regardless of how it was created.
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.
Re-worked section to hopefully including this
|
|
||
| A profile should be treated as immutable, so once created it cannot be updated. | ||
| Updating a profile would cause mismatches with flows which have been already created using that profile and so breaks the model for profiles. | ||
| To update a profile a new one with a new ID should be created. |
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.
Do we (or should we) have an option to "archive" an "old" Profile that should no longer be used but is in use already and therefore cannot easily be deleted?
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.
Currently this is not in scope of this PR. We do need to think about old profiles, however we should also consider the same approach for the storage backend endpoint under the services section also. So previously proposed that we handle both of these at the same time in a future PR
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.
For completeness, one option is to add fine-grained auth to profiles (and storage backends) to allow their use to be restricted as required.
| Additionally if the decision is made to change a standard profile in the future then this does not affect the existing flows as the parameters have already been replicated and should remain unchanged to reflect the media actually being stored. | ||
|
|
||
| Flows that have been created from a profile will include the parameter indicating which profile they were created from. | ||
| This differentiates them from the flows that have been created with the technical characteristics directly. |
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.
Do we need to differentiate between flows that were created using a Profile and a Flow created using exactly the same Metadata as an existing Profile?
If we didn't then perhaps the store should detect this at creation time and set the Profile Id to match even if not supplied?
OR it could reject the request saying that this Flow "metadata" matches an existing Profile and therefore the profile should be used?
Just thinking out loud here.
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.
This probably needs the outcome of the greedy match discussion to complete. If we're not going to implement greedy match at any level then the only way the profile_id field should be populated is if it's provided at creation.
If we want to do match on creation and populate the profile field then yes we may want to indicate this. However the question is what use is this in the actual TAMS metadata or whether it should be captured in an observability layer for reporting/tracking.
I would probably stay away from rejecting requests where they exactly match a profile. I would either match on creation and populate the field or leave creators the choice to use profiles or not as they wish.
| The edit process could then process each input source in turn read the available flows and match them to the destination flows using the profile tag easily. | ||
| If the source content has additional non standard flows then these could be ignored. | ||
|
|
||
| From a flow created from a profile, it is possible to query via both profile ID and also the individual parameter of the flow that have been inheritied from the profile. |
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.
We should define the behaviour if these conflict. I.e. You have a profile A that defines codec as X. You query for Flows that use Profile A using the new query parameter but also use the existing codec query parameter and set that to Y. What should the behaviour be?
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.
Re-worked section to hopefully including this
Details
Add the content of flow profiles to aid with the creation of flows in stores where there are house formats being used. Also significantly simplifies matching flows across edit by reference workflows
Jira Issue (if relevant)
Jira URL:
Related PRs
If merged before #115, add the new endpoints to the auth logic listings in that PR. If merged after, add that logic in this PR
Submitter PR Checks
(tick as appropriate)
Reviewer PR Checks
(tick as appropriate)
Info on PRs
The checks above are guidelines. They don't all have to be ticked, but they should all have been considered.