Page MenuHomeSoftware Heritage
Differential D380

Reject deposit containing a single archive
ClosedPublic
Actions

Authored by ardumont on Jul 13 2018, 5:39 PM.

Details

Reviewers
moranegg
Commits
rDDEP6367c2edc7ba: deposit_check: Clarify docstring
rDDEPd475ce2c7fa6: test_deposit_check: Add more tests around invalid archive scenario
rDDEP867e24aee768: private/deposit_check: Regroup error message together
rDDEP490e188549fa: deposit_status: Update the deposit status endpoint
rDDEPe5b9164e53a0: swh.deposit.models: Reuse status variable
rDDEP23850ff89715: test_deposit_check: Add missing test case (missing archive)
rDDEP5ad0e510991d: docs: Update deposit with status rejected documentation
rDDEPf29e3f49ad97: deposit_check: Add test around deposit with 1 archive rejection
rDDEPfd3d9085693f: deposit_check: Reject deposit containing a single archive
Summary

deposit_check: Reject deposit containing a single archive
deposit_check: Add test around deposit with 1 archive rejection

Related T1123

Test Plan

Tests ok

Diff Detail

Repository
rDDEP Push deposit
Branch
reject-invalid-deposit
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1342
Build 1686: arc lint + arc unit

Event Timeline

ardumont created this revision.Jul 13 2018, 5:39 PM
Harbormaster completed remote builds in B1321: Diff 1181.Jul 13 2018, 5:39 PM
ardumont updated this revision to Diff 1183.Jul 13 2018, 5:57 PM

Fix spurrious print statements

Harbormaster completed remote builds in B1323: Diff 1183.Jul 13 2018, 5:57 PM
ardumont updated this revision to Diff 1184.Jul 13 2018, 6:09 PM

Update deposit status endpoint

As multiple checks on archive can happen now, the structure slightly change.

Fortunately no deposit have that checks failing for now so no migration needed.

Harbormaster completed remote builds in B1324: Diff 1184.Jul 13 2018, 6:09 PM
ardumont updated this revision to Diff 1185.Jul 14 2018, 11:04 AM

Amend commit

Harbormaster completed remote builds in B1325: Diff 1185.Jul 14 2018, 11:04 AM
moranegg added a comment.Jul 16 2018, 11:10 AM

My only comment would be to better document the possible archive errors that appear in the test,
if there are 4 types of errors -> missing / unreadable / unsupported / invalid, it will be easier to understand where they are captured if documented.

ardumont added a comment.Jul 16 2018, 11:59 AM

My only comment would be to better document the possible archive errors that appear in the test,
if there are 4 types of errors -> missing / unreadable / unsupported / invalid, it will be easier to understand where they are captured if documented.

Ok, where do you propose we add that?
I see docs/endpoints/status.rst which seems like a good idea.

ardumont mentioned this in D381: Refactor common reading behavior between deposit read and check.Jul 16 2018, 3:35 PM
ardumont updated this revision to Diff 1202.Jul 16 2018, 3:51 PM

Update according to latest remarks

  • swh.deposit.models: Reuse status variable
  • docs: Update deposit with status rejected documentation
ardumont updated this revision to Diff 1203.Jul 16 2018, 4:40 PM

Updates

  • test_deposit_check: Add missing test case (missing archive)
  • private/deposit_check: Regroup error message together
moranegg added a comment.Jul 17 2018, 12:06 PM

Looks good to me.
All tests pass...

swh/deposit/api/private/deposit_check.py
76–77

yes that's good.
on the third check i would add the name valid (as the opposite of invalid):

  • valid content: the archive does not contain a single archive file
moranegg accepted this revision.Jul 17 2018, 12:06 PM
This revision is now accepted and ready to land.Jul 17 2018, 12:06 PM
ardumont updated this revision to Diff 1204.Jul 17 2018, 12:23 PM
  • test_deposit_check: Add more tests around invalid archive scenario
ardumont updated this revision to Diff 1205.Jul 17 2018, 12:32 PM
  • deposit_check: Clarify docstring
ardumont marked an inline comment as done.Jul 17 2018, 12:32 PM
moranegg accepted this revision.Jul 17 2018, 12:44 PM
moranegg added inline comments.
swh/deposit/api/private/deposit_check.py
28

I like this new approach

ardumont added inline comments.Jul 17 2018, 1:22 PM
swh/deposit/tests/api/test_deposit_check.py
106

I like this new approach

Cool.
Also, note that i wanted to use it that ARCHIVE_EXTENSION variable content to fully check all here.
But that will:

  • complexify the code to compress the 'invalid' archive (see code below)
  • also probably lengthen the tests period run

That's why i only limited to those (for now ;)

swh/deposit/tests/common.py
109

Here is the code to compress an archive according to latest remark.

ardumont added inline comments.Jul 17 2018, 1:23 PM
swh/deposit/tests/common.py
109

latest remark is D380#inline-1900 ;)

ardumont closed this revision.Jul 17 2018, 1:26 PM
Harbormaster completed remote builds in B1342: Diff 1202.Jul 17 2018, 2:15 PM
Harbormaster completed remote builds in B1343: Diff 1203.
Harbormaster completed remote builds in B1344: Diff 1204.
Harbormaster completed remote builds in B1345: Diff 1205.

Revision Contents
Changeset List

PathSize
docs/
endpoints/
52 lines
swh/
deposit/
api/
36 lines
private/
72 lines
12 lines
tests/
api/
50 lines
20 lines
78 lines

Diff 1202

View Options

docs/endpoints/status.rst

Loading...
View Options

swh/deposit/api/deposit_status.py

Loading...
View Options

swh/deposit/api/private/deposit_check.py

Loading...
View Options

swh/deposit/models.py

Loading...
View Options

swh/deposit/tests/api/test_deposit_check.py

Loading...
View Options

swh/deposit/tests/api/test_deposit_status.py

Loading...
View Options

swh/deposit/tests/common.py

Loading...
About · Developer information · Legal