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
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19849
Build 30825: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 30824: arc lint + arc unit

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
752

Why such formulation?

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

779

Missing test case.

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

do we :D ?

522
675

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
752

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
49

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

522

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

675

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

swh/deposit/api/common.py
779

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
751–752

then ^ is even clearer and without any ambiguity.

779

then remove it?

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

it was a joke ;)
sorry for misleading you.

522

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
779

I'll make it a 500 error

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

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
779

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.