-
Notifications
You must be signed in to change notification settings - Fork 6
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
Expand list model to include fields required by Mini profiles and Multi-byline formats #247
Conversation
This won't be used yet but will save having to add it later
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!
@simonbyford has published a preview version of this PR with release workflow run #66, based on commit 816f303: 25.0.0-PREVIEW.expand-list-model.2024-06-14T0911.816f303c Want to make another preview release?Click 'Run workflow' in the GitHub UI, specifying the expand-list-model branch, or use the GitHub CLI command: gh workflow run release.yml --ref expand-list-model Want to make a full release after this PR is merged?Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command: gh workflow run release.yml |
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 looks fine to me :)
For the record/history, this PR caused us a small bit of trouble! Because https://github.com/guardian/content-api/pull/2879 updated porter to write the contributors as a list of IDs, defining the type of those contributors to be a list of tag objects here is effectively imposing a requirement on concierge to transform those ids into tag objects, or drop them. (This is because this model defines the structure of the data that concierge must output based on what it gets from elasticsearch.) Since we never updated concierge to do this, once we got one of these fields in CODE (while testing https://github.com/guardian/content-api/pull/2925), concierge was unable to handle the field and broke whenever it tried to output the content containing the field. (This also broke crier, since it calls concierge.) I’ll keep this in mind in future when reviewing PRs like this, and possibly we should require tests of some sort on this repository that would catch this – I’m not sure what those would be though. It’s also worth noting that since the flexible-model defines the structure of the data accepted by porter, which it then transforms before writing it into elasticsearch, we don’t necessarily want the two models to be aligned: only where porter makes no changes before writing to elasticsearch. We should also consider updating concierge (and crier?) to continue operating well when they encounter an error like this. (Something else I’d like to look into is having another model that’s shared between porter and concierge and represents the data written to elasticsearch. To preserve the ability to make changes, we’d need to have tests that new versions of the library can’t write data that breaks old versions of the library, but that seems like a plausible feature. (For example, ophan’s avro-logger has similar tests which make sure new schema versions can always read data written by old versions.)) |
What does this change?
Expands the list model to include fields required by Mini profiles and Multi-byline formats. This brings the model into line with flexible-model: https://github.com/guardian/flexible-model/pull/69
How to test
How can we measure success?
Have we considered potential risks?
Images
Accessibility