-
Notifications
You must be signed in to change notification settings - Fork 214
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
Expose provider in the API tags response #4280
Conversation
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.
Nice that the change is so small! Thank you for the explanation on allow_null
. I wonder if this is only this way in the local data, or in prod, too?
Because it has to do with what we index into Elasticsearch, I assume it would affect production as well. The local data is "complete" in the sense that all of the tags have a provider in the actual database. If anything, I would imagine production to be less compliant on that front! |
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 great!
Co-authored-by: zack <6351754+zackkrida@users.noreply.github.com>
* Expose provider in TagsSerializer * Add some machine-generated labels to sample data * Update documentation examples * Fix some tests * Allow the provider field to be nullable for ES compatibility * Update phrasing Co-authored-by: zack <6351754+zackkrida@users.noreply.github.com> --------- Co-authored-by: zack <6351754+zackkrida@users.noreply.github.com>
Fixes
Fixes #4273 by @AetherUnbound
Description
This PR exposes the
provider
field within the tags attribute that is present in the database under the keyunstable__provider
. The core of this change is inmedia_serializers.py
, most of the other changes in this PR revolve around making sure the field shows up in tests and documentation.One thing that was unexpected was having to set
allow_null=True
on the serializer. It seems like there are some cases where we're converting results directly from Elasticsearch into serialized results rather than retrieving the full data from the database. We could alter the ingestion server to ingest this new field, but I don't think it's necessary (and I think we can leave this as nullable here, given the explanation I've provided).Testing Instructions
Tests should pass! You can also do the following:
just down -v
just api/init
unstable__provider
keyChecklist
Update index.md
).main
) or a parent feature branch.just catalog/generate-docs
for catalogPRs) or the media properties generator (
just catalog/generate-docs media-props
for the catalog or
just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin