Page MenuHomeSoftware Heritage
Differential D3253

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

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

Details

Reviewers
douardda
Group Reviewers
Reviewers
Maniphest Tasks
T2310: Make origin visits immutable
Commits
rDLDBASE65eb70d7b704: loader: Migrate to origin visit status
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
Branch
use-ovs-add
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12747
Build 19388: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 19387: arc lint + arc unit

Event Timeline

ardumont created this revision.Jun 10 2020, 12:36 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJun 10 2020, 12:36 PM
swh-public-ci added a comment.Jun 10 2020, 12:38 PM

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

Harbormaster failed remote builds in B12742: Diff 11538!Jun 10 2020, 12:38 PM
ardumont updated this revision to Diff 11540.Jun 10 2020, 1:02 PM

Rebase on local master without issue

Depends on D3254

ardumont edited the summary of this revision. (Show Details)Jun 10 2020, 1:03 PM
swh-public-ci added a comment.Jun 10 2020, 1:03 PM

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.

Harbormaster completed remote builds in B12744: Diff 11540.Jun 10 2020, 1:03 PM
ardumont updated this revision to Diff 11543.Jun 10 2020, 1:21 PM

Rebase on latest master

ardumont edited the summary of this revision. (Show Details)Jun 10 2020, 1:21 PM
swh-public-ci added a comment.Jun 10 2020, 1:22 PM

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.

Harbormaster completed remote builds in B12747: Diff 11543.Jun 10 2020, 1:23 PM
douardda added a subscriber: douardda.Jun 10 2020, 2:22 PM
douardda added inline comments.
swh/loader/package/loader.py
287

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)

douardda added inline comments.Jun 10 2020, 2:24 PM
swh/loader/package/loader.py
287

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).

douardda accepted this revision.Jun 10 2020, 2:25 PM

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
ardumont added inline comments.Jun 10 2020, 2:27 PM
swh/loader/package/loader.py
287

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 ;)

ardumont added inline comments.Jun 10 2020, 2:34 PM
swh/loader/package/loader.py
287

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

ardumont updated this revision to Diff 11547.Jun 10 2020, 2:42 PM

Reuse snapshot_id variable now that it's defined

swh-public-ci added a comment.Jun 10 2020, 2:44 PM

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.

Harbormaster completed remote builds in B12751: Diff 11547.Jun 10 2020, 2:44 PM
ardumont added inline comments.Jun 10 2020, 2:57 PM
swh/loader/package/loader.py
287

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

ardumont mentioned this in T2310: Make origin visits immutable.Jun 10 2020, 3:09 PM
ardumont updated this revision to Diff 11560.EditedJun 11 2020, 11:18 AM

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"
swh-public-ci added a comment.Jun 11 2020, 11:19 AM

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.

Harbormaster completed remote builds in B12764: Diff 11560.Jun 11 2020, 11:19 AM
Closed by commit rDLDBASE65eb70d7b704: loader: Migrate to origin visit status (authored by ardumont). · Explain WhyJun 11 2020, 11:36 AM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLDBASE65eb70d7b704: loader: Migrate to origin visit status.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
loader/
core/
22 lines81%
Loading...
package/
15 lines93%
Loading...

Diff 11543

View Options

swh/loader/core/loader.py

Loading...
View Options

swh/loader/package/loader.py

Loading...
About · Developer information · Legal