Page MenuHomeSoftware Heritage

Change `url` and `external-id` from mandatory to optional metadata
ClosedPublic

Authored by ardumont on Apr 16 2019, 7:09 PM.

Details

Summary
  • deposit_read: Compute origin-url from the deposit's setup
  • check: Remove url and external_identifier from mandatory fields

Related T1648

Test Plan

tox

Diff Detail

Repository
rDDEP Push deposit
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

swh/deposit/utils.py
22–23

More a question than a request of change (because I'm new to this code base), but where in this change we verify that *an* external_id is actually present and not simply missing? It is totally right to make external_id optional *as a metadata field*, but it should not be missing entirely---the client should be providing an UUID if it's not being provided by other means. In that case, though, the code that generate the complete URL should check that, one way or another, *an* external_id is present, as a safety net.

Fix in-between interactive rebase

swh/deposit/utils.py
22–23

More a question than a request of change (because I'm new to this code base), but where in this change we verify that *an* external_id is actually present and not simply missing?

So far, we don't check.

In the current workflow with hal, it is provided (and i don't expect that to change because the slug is still mandatory on the api).
Also, with the deposit client cli, we do generate it if not provided (and then associating it to the deposit).
So i expect this to be always present...

I can add the check but i'm not sure what to do if the cases arise, raising?

zack requested changes to this revision.Apr 17 2019, 11:32 AM
zack added inline comments.
swh/deposit/utils.py
22–23

My point is that *today* we are sure it's around, but tomorrow the code that make sure the ID is there might be changed and, when that happens, we will not notice that is missing and will start filling the archive with URLs that only contain the domain name and no ID. Please add the check (an assertion would be enough) as a classic defensive programming measure.

This revision now requires changes to proceed.Apr 17 2019, 11:32 AM
  • utils: Enforce necessary setup for deposit and associated client
This revision is now accepted and ready to land.Apr 17 2019, 11:50 AM
This revision was automatically updated to reflect the committed changes.