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 swh-deposit
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Apr 16 2019, 7:09 PM
zack added inline comments.Apr 17 2019, 10:14 AM
swh/deposit/utils.py
23–24

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.

ardumont updated this revision to Diff 4605.Apr 17 2019, 10:45 AM

Rebase on dedicated branch

ardumont updated this revision to Diff 4609.Apr 17 2019, 10:53 AM

Rebase on latest master

ardumont updated this revision to Diff 4610.Apr 17 2019, 11:00 AM

Fix in-between interactive rebase

ardumont added inline comments.Apr 17 2019, 11:29 AM
swh/deposit/utils.py
23–24

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
23–24

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
ardumont updated this revision to Diff 4611.Apr 17 2019, 11:46 AM
  • utils: Enforce necessary setup for deposit and associated client
zack accepted this revision.Apr 17 2019, 11:50 AM
This revision is now accepted and ready to land.Apr 17 2019, 11:50 AM
ardumont updated this revision to Diff 4612.Apr 17 2019, 11:51 AM

Plug to master

This revision was automatically updated to reflect the committed changes.