-
Notifications
You must be signed in to change notification settings - Fork 212
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
Warn on license_url
computation in the API
#4198
Conversation
104a518
to
1790189
Compare
@krysal what is the status of this PR and this particular blocker? Is the PR still blocked? Should it be labelled as such?
|
@krysal, what if instead of removing the computation completely we leave it in but add a warning log. Then, if there are no warnings in a longish period of time (a month?), we can remove the computation? |
@sarayourfriend It's blocked pending completion of #3885, which I thought would be a relatively quick task, but some problems arose.
That is a good idea! :) I'll adapt the PR for that 👍 |
1790189
to
4a3a800
Compare
license_url
computation in apilicense_url
computation in the API
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, I was able to see the log after removing the meta_data from a record locally. It's been a minute since I've looked at this -- can you remind me what cases we're trying to catch here? I thought the catalog should be enforcing setting the licenses now, and that incorrect cases had already been backfilled, but maybe I'm misremembering.
If there isn't one already, can we make an issue for checking the production logs so that it does not get forgotten?
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.
LGTM! Added a suggestion to make logging more structured but it's not a blocker in my view.
@stacimc I'm also hoping we don't see more incorrect cases but I created #4318 to wait and confirm that. There is also more explanation of the reasoning. |
ddffa95
to
cdb7aa3
Compare
Co-authored-by: Dhruv Bhanushali <[email protected]>
cdb7aa3
to
06437e3
Compare
Fixes
Related to #703 by @obulat
Description
In the interest of closing this PR, I changed the approach only to warn when the license is computed, as suggested by @obulat. The #703 issue will addressed when we confirm #4318 is not an issue anymore.
Old description
My understanding is that the computation from
openverse_attribution.license.License
can be removed but we still need the property.This PR should not be merged until the updated
add_license_url
run is completed and it's confirmed theimage
table in the Catalog does not have moreNULL
values for this field.If you are wondering about
audio
I confirmed all items have this field set to something else thanNULL
.Testing Instructions
Run the API and remove the license_url of an image. Then, visit its media page and check the logs.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin