Page MenuHomeSoftware Heritage

Port cgit lister to the new lister api
ClosedPublic

Authored by vsellier on Jan 22 2021, 3:53 PM.

Details

Summary

Related to T2984

Test Plan

tox

Note:

  • test inputs were left as-is
  • tests were slightly modified due to the scheduler api change (for reading)

Diff Detail

Repository
rDLS Listers
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18669
Build 28891: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 28890: arc lint + arc unit

Event Timeline

Build has FAILED

Patch application report for D4926 (id=17533)

Rebasing onto ff232f0d91...

Current branch diff-target is up to date.
Changes applied before test
commit 8bdb35311e8022abf4cb8065b034391b51fa6cb6
Author: Vincent SELLIER <vincent.sellier@softwareheritage.org>
Date:   Fri Jan 22 15:39:49 2021 +0100

    Port cgit lister to the new lister api
    
    Related to T2984

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

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 22 2021, 3:56 PM
Harbormaster failed remote builds in B18665: Diff 17533!
vsellier edited the test plan for this revision. (Show Details)

fix tests

Build is green

Patch application report for D4926 (id=17537)

Rebasing onto ff232f0d91...

Current branch diff-target is up to date.
Changes applied before test
commit 27feeb2122dd3125fcfe01df799bc8cfdea312ec
Author: Vincent SELLIER <vincent.sellier@softwareheritage.org>
Date:   Fri Jan 22 15:39:49 2021 +0100

    Port cgit lister to the new lister api
    
    Related to T2984

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

anlambert added a subscriber: anlambert.
anlambert added inline comments.
swh/lister/cgit/lister.py
38

missing types for url and instance parameters

52

Here instance should not be equal to None as it is used to create the internal lister id.

Two possible solutions to ensure it:

  • make the instance parameter of the lister and celery tasks non optional
  • set the instance value to the url one
108

It would be nice to compute a last_update value for the listed origins.

On the main Web UI page of a cgit instance listing hosted repositories, there is an Idle column indicating
when a repository was updated relative to the current date. This should not be complicated to parse
and a datetime object could be produced from it.

swh/lister/cgit/tests/test_lister.py
11

test name is ambiguous as get_pages is called in implementation.

28

test should be renamed here

53

return value of the run method should be used to test the number of pages and listed origins

This revision now requires changes to proceed.Jan 25 2021, 11:51 AM
swh/lister/cgit/lister.py
108

this is not in a scope of that diff, just throwing the idea here ;-)

swh/lister/cgit/lister.py
52

Sorry, I did not see the instructions above, that comment is not valid.

swh/lister/cgit/lister.py
108

exact, we have changed at iso-functionalities.
I have created the T2988 to not forget it.

  • rebase
  • update tests according to the review feedbacks
ardumont added inline comments.
swh/lister/tests/test_cli.py
74

For those wondering, this test is no longer relevant as those listers are no longer exposing the conf_override parameter in their constructor.

Build is green

Patch application report for D4926 (id=17578)

Rebasing onto 59c9abb916...

Current branch diff-target is up to date.
Changes applied before test
commit e4a590fc7f3e9f95c756e616437ee29cf734582f
Author: Vincent SELLIER <vincent.sellier@softwareheritage.org>
Date:   Fri Jan 22 15:39:49 2021 +0100

    Port cgit lister to the new lister api
    
    Related to T2984

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

This revision is now accepted and ready to land.Jan 25 2021, 3:06 PM
This revision was automatically updated to reflect the committed changes.
tenma added inline comments.
swh/lister/cgit/lister.py
38

lister url should not be optional, even if it is required in the task : it cannot function without one.

62

add a warning on any error != than 200 that dumps response content, like the other listers, to ease debugging.

89

Here I think the lister will stop without error if the site changes, but in this case we want it to fail clearly and not just end.

swh/lister/cgit/lister.py
62