Page MenuHomeSoftware Heritage

loader: Migrate to use origin visit status add storage endpoint
ClosedPublic

Authored by ardumont on Jun 10 2020, 12:36 PM.

Details

Summary

Note that this means it stops creating origin visits in the storage backend.

Which regarding reading origin_visit changes nothing as this uses origin_visit_status anyway.

Related to T2310

Test Plan

tox

Diff Detail

Repository
rDLDBASE Generic VCS/Package Loader
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 D3253 (id=11538)

Rebasing onto c28799f674...

Current branch diff-target is up to date.
Changes applied before test
commit 5bc737d34655e7742f07c770769bd0bb10386cc7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Jun 9 17:39:58 2020 +0200

    loader: Migrate to origin visit status
    
    Related to T2310

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

Rebase on local master without issue

Depends on D3254

Build is green

Patch application report for D3253 (id=11540)

Could not rebase; Attempt merge onto c28799f674...

Updating c28799f..5df8a7b
Fast-forward
 requirements-swh.txt                             |  2 +-
 swh/loader/core/loader.py                        | 22 ++++++++++++++--------
 swh/loader/package/deposit/tests/test_deposit.py | 14 +++++++-------
 swh/loader/package/loader.py                     | 15 +++++++++++----
 4 files changed, 33 insertions(+), 20 deletions(-)
Changes applied before test
commit 5df8a7b5ebca3d8dfc1985428ae6fa21730a93cb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Jun 9 17:39:58 2020 +0200

    loader: Migrate to origin visit status
    
    Related to T2310

commit 810073dd947181aa69d9f75922492bbf3cf86e88
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jun 10 13:00:05 2020 +0200

    test_deposits: Fix origin_metadata_get which is a paginated endpoint

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

Build is green

Patch application report for D3253 (id=11543)

Rebasing onto 39e050b763...

Current branch diff-target is up to date.
Changes applied before test
commit f57c29909129a4649a1947e358172da932a99534
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Jun 9 17:39:58 2020 +0200

    loader: Migrate to origin visit status
    
    Related to T2310

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

douardda added inline comments.
swh/loader/package/loader.py
286

not that it would make any difference I guess, but why keep the and snapshot.id part in here?

(also not sure this local variable is helping really)

swh/loader/package/loader.py
286

I mean, it adds 3 lines of code that are not necessary not really ease to read or understand this piece of code. (once again, IMHO. Others may disagree).

lgtm but see my comments, in case you agree with them.

This revision is now accepted and ready to land.Jun 10 2020, 2:25 PM
swh/loader/package/loader.py
286

I'm not sure either.
mypy annoyed me and it finally left me alone after this... as far as i recall what happened.
I'll try to simplify now that one day passed in between ;)

swh/loader/package/loader.py
286

Although from the model Snapshot, the id field can be "" (as a default value) so I guess, that's needed to avoid this case.

Reuse snapshot_id variable now that it's defined

Build is green

Patch application report for D3253 (id=11547)

Rebasing onto 39e050b763...

Current branch diff-target is up to date.
Changes applied before test
commit 1191cafdb1b85f87a586163826436a7d2c7c8403
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Jun 9 17:39:58 2020 +0200

    loader: Migrate to origin visit status
    
    Related to T2310

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

swh/loader/package/loader.py
286

well i meant`b""` (so the type is correct :).
Still that's not a correct id we want to store.

Add comment as to why we use the intermediary variable (sole ok solution i found)

The other ways i tried, mypy is unhappy:

  • assigning directly (, snapshot=snapshot and snapshot.id) [1]
  • using snapshot_id = snapshot.id if snapshot and snapshot.id else None [2]

[1]

swh/loader/package/loader.py:294: error: Argument "snapshot" to "OriginVisitStatus" has incompatible type "Union[Snapshot, None, bytes]"; expected "Optional[bytes]"

[2]

swh/loader/package/loader.py:289: error: "None" has no attribute "id"

Build is green

Patch application report for D3253 (id=11560)

Rebasing onto 39e050b763...

Current branch diff-target is up to date.
Changes applied before test
commit 65eb70d7b704204b327bc45900d2eeef40c336e7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Jun 9 17:39:58 2020 +0200

    loader: Migrate to origin visit status
    
    Related to T2310

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