Page MenuHomeSoftware Heritage

Check a SWHID exists in the archive before accepting a metadata-only deposit
ClosedPublic

Authored by vlorentz on Mar 11 2021, 5:29 PM.

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

Build has FAILED

Patch application report for D5231 (id=18753)

Rebasing onto 4353a323f6...

Current branch diff-target is up to date.
Changes applied before test
commit 7f0e861415ee4888fa8fedf7a2c91395415f468d
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Mar 11 17:29:12 2021 +0100

    Check a SWHID exists in the archive before accepting a metadata-only deposit

Link to build: https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/555/
See console output for more information: https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/555/console

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 11 2021, 5:30 PM
Harbormaster failed remote builds in B19849: Diff 18753!
  • make flake8 happy
  • move checks to their own function

Build is green

Patch application report for D5231 (id=18767)

Rebasing onto 4353a323f6...

Current branch diff-target is up to date.
Changes applied before test
commit bb053c7bd51bbd393db9bde991dfa9ffa1e7e202
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Mar 11 17:29:12 2021 +0100

    Check a SWHID exists in the archive before accepting a metadata-only deposit

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/562/ for more details.

ardumont added a subscriber: ardumont.

lgtm

required changes for the sake of some remarks/questions inline

swh/deposit/api/common.py
717

Why such formulation?

just a plain yet is enough (same goes for the next occurrence ;)

744

Missing test case.

swh/deposit/tests/api/test_collection_post_atom.py
48

do we :D ?

521
674

isn't it missing a swhid on origin in there ^

maybe not, oh well, please do tell ;)

This revision now requires changes to proceed.Mar 12 2021, 2:15 PM
swh/deposit/api/common.py
717

Because "does not exist in the archive yet" may sound like it is about to be loaded

swh/deposit/tests/api/test_collection_post_atom.py
48

I just want *any* object with that swhid, I don't care if the examples are reproducible

521

It is not necessarily the reason. It's just an assertion anyway, the code will be show next to the message

674

That would be a different error msg (the one not tested as you noted)

swh/deposit/api/common.py
744

Actually, that code is unreachable, because it's an ExtendedSWHID converted from a QualifiedSWHID, so swh:1:ori and swh:1:emd are caught ahead of that.

add test checking that extended swhid types are rejected

Build is green

Patch application report for D5231 (id=18778)

Rebasing onto 4353a323f6...

Current branch diff-target is up to date.
Changes applied before test
commit 964c992ebe52c093e6b91819683f8b47dd9245c0
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Mar 11 17:29:12 2021 +0100

    Check a SWHID exists in the archive before accepting a metadata-only deposit

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/567/ for more details.

swh/deposit/api/common.py
716–717

then ^ is even clearer and without any ambiguity.

744

then remove it?

swh/deposit/tests/api/test_collection_post_atom.py
48

it was a joke ;)
sorry for misleading you.

521

ok, but the plain extra parameter is not quite clear to me.

So i'd like to have a prefix in the message the assertion to explicit it: f"Actual response: ..." may be better here.

swh/deposit/api/common.py
744

I'll make it a 500 error

swh/deposit/tests/api/test_collection_post_atom.py
521

It's just a regular assertion...

and I also don't want to add two lines on every assertion

ardumont added inline comments.
swh/deposit/api/common.py
744

ok then (i gather in another diff ;)

This revision is now accepted and ready to land.Mar 13 2021, 10:54 AM

raise 500 instead of 400 when _check_swhid_in_archive gets an unexpected SWHID type

This revision was landed with ongoing or failed builds.Mar 15 2021, 10:26 AM
This revision was automatically updated to reflect the committed changes.

Build is green

Patch application report for D5231 (id=18797)

Rebasing onto 4353a323f6...

Current branch diff-target is up to date.
Changes applied before test
commit c4972584f176b36c804c4c96bacf4ee60b40effb
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Mar 11 17:29:12 2021 +0100

    Check a SWHID exists in the archive before accepting a metadata-only deposit

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/572/ for more details.