Page MenuHomeSoftware Heritage
Differential D4926

Port cgit lister to the new lister api
ClosedPublic
Actions

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

Details

Reviewers
anlambert
Group Reviewers
Reviewers
Maniphest Tasks
T2984: Port cgit lister to the new Lister API
Commits
rDLSe4a590fc7f3e: Port cgit lister to the new lister api
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 18665
Build 28883: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 28882: arc lint + arc unit

Unit TestsFailed
View All

TimeTest
1,327 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.lister.tests.test_cli::test_get_lister
swh_scheduler_config = {'db': "dbname=scheduler user=postgres host=127.0.0.1 port=32432 options=''"} def test_get_lister(swh_scheduler_config):
1,327 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.lister.tests.test_cli::test_get_lister_override
def test_get_lister_override(): """Overriding the lister configuration should populate its config
185 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.lister.bitbucket.tests.test_lister::test_bitbucket_full_lister
199 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.lister.bitbucket.tests.test_lister::test_bitbucket_incremental_lister
273 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.lister.bitbucket.tests.test_lister::test_bitbucket_lister_rate_limit_hit
View Full Test Results (2 Failed · 126 Passed)

Event Timeline

vsellier created this revision.Jan 22 2021, 3:53 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJan 22 2021, 3:53 PM
swh-public-ci added a comment.Jan 22 2021, 3:56 PM

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!
ardumont edited the test plan for this revision. (Show Details)Jan 22 2021, 3:56 PM
ardumont added projects: Lister, Sprint 2021 01.
vsellier updated this revision to Diff 17537.Jan 22 2021, 4:21 PM
vsellier edited the test plan for this revision. (Show Details)

fix tests

swh-public-ci added a comment.Jan 22 2021, 4:24 PM

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.

Harbormaster completed remote builds in B18669: Diff 17537.Jan 22 2021, 4:24 PM
vsellier requested review of this revision.Jan 22 2021, 4:24 PM
anlambert requested changes to this revision.Jan 25 2021, 11:51 AM
anlambert added a subscriber: anlambert.
anlambert added inline comments.
swh/lister/cgit/lister.py
39

missing types for url and instance parameters

59

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
109

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–13

test name is ambiguous as get_pages is called in implementation.

27

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
anlambert added inline comments.Jan 25 2021, 11:54 AM
swh/lister/cgit/lister.py
109

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

anlambert added inline comments.Jan 25 2021, 11:57 AM
swh/lister/cgit/lister.py
59

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

vsellier added inline comments.Jan 25 2021, 12:17 PM
swh/lister/cgit/lister.py
109

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

vsellier updated this revision to Diff 17578.Jan 25 2021, 2:58 PM
  • rebase
  • update tests according to the review feedbacks
ardumont added a subscriber: ardumont.Jan 25 2021, 3:00 PM
ardumont added inline comments.
swh/lister/tests/test_cli.py
73 ↗(On Diff #17578)

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

swh-public-ci added a comment.Jan 25 2021, 3:01 PM

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.

Harbormaster completed remote builds in B18713: Diff 17578.Jan 25 2021, 3:01 PM
anlambert accepted this revision.Jan 25 2021, 3:06 PM

Looks good to me !

This revision is now accepted and ready to land.Jan 25 2021, 3:06 PM
Closed by commit rDLSe4a590fc7f3e: Port cgit lister to the new lister api (authored by vsellier). · Explain WhyJan 25 2021, 3:06 PM
This revision was automatically updated to reflect the committed changes.
vsellier added a commit: rDLSe4a590fc7f3e: Port cgit lister to the new lister api.
tenma added a subscriber: tenma.Jan 25 2021, 4:58 PM
tenma added inline comments.
swh/lister/cgit/lister.py
39

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

70

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

91

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.

tenma added inline comments.Jan 25 2021, 5:02 PM
swh/lister/cgit/lister.py
70
vsellier mentioned this in D4943: cgit lister: Add missing types on the init method.Jan 25 2021, 6:33 PM

Revision Contents
Changeset List

PathSize
swh/
lister/
cgit/
4 lines
141 lines
9 lines
tests/
84 lines
22 lines

Diff 17533

View Options

swh/lister/cgit/__init__.py

Loading...
View Options

swh/lister/cgit/lister.py

Loading...
View Options

swh/lister/cgit/models.py

Loading...
View Options

swh/lister/cgit/tasks.py

Loading...
View Options

swh/lister/cgit/tests/conftest.py

Loading...
View Options

swh/lister/cgit/tests/test_lister.py

Loading...
View Options

swh/lister/cgit/tests/test_tasks.py

Loading...
About · Developer information · Legal