Page MenuHomeSoftware Heritage

Reject deposit containing a single archive
ClosedPublic

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

Diff Detail

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

Event Timeline

Fix spurrious print statements

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.

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.

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.

Update according to latest remarks

  • swh.deposit.models: Reuse status variable
  • docs: Update deposit with status rejected documentation

Updates

  • test_deposit_check: Add missing test case (missing archive)
  • private/deposit_check: Regroup error message together

Looks good to me.
All tests pass...

swh/deposit/api/private/deposit_check.py
83–84

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
This revision is now accepted and ready to land.Jul 17 2018, 12:06 PM
  • test_deposit_check: Add more tests around invalid archive scenario
  • deposit_check: Clarify docstring
moranegg added inline comments.
swh/deposit/api/private/deposit_check.py
35

I like this new approach

swh/deposit/tests/api/test_deposit_check.py
68

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
82

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

swh/deposit/tests/common.py
82

latest remark is D380#inline-1900 ;)

Harbormaster completed remote builds in B1343: Diff 1203.
Harbormaster completed remote builds in B1344: Diff 1204.
Harbormaster completed remote builds in B1345: Diff 1205.