Resolves T2942
Details
- Reviewers
ardumont - Group Reviewers
Reviewers - Maniphest Tasks
- T2942: Update deposit: `swhid` MUST exist in archive for a metadata-only deposit
- Commits
- rDDEPc4972584f176: Check a SWHID exists in the archive before accepting a metadata-only deposit
Diff Detail
- Repository
- rDDEP Push deposit
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 19863 Build 30851: Phabricator diff pipeline on jenkins Jenkins console · Jenkins Build 30850: 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
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.
lgtm
required changes for the sake of some remarks/questions inline
swh/deposit/api/common.py | ||
---|---|---|
718 | Why such formulation? just a plain yet is enough (same goes for the next occurrence ;) | |
745 | Missing test case. | |
swh/deposit/tests/api/test_collection_post_atom.py | ||
49 | do we :D ? | |
521 | ||
675 | isn't it missing a swhid on origin in there ^ maybe not, oh well, please do tell ;) |
swh/deposit/api/common.py | ||
---|---|---|
718 | 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 | |
521 | 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 | ||
---|---|---|
745 | 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. |
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. | |
745 | then remove it? | |
swh/deposit/tests/api/test_collection_post_atom.py | ||
49 | it was a joke ;) | |
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 | ||
---|---|---|
745 | ok then (i gather in another diff ;) |
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.