Paths

Table of Contentst

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 18669
Build 28891: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 28890: arc lint + arc unit

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

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.

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.

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

anlambert added inline comments.Jan 25 2021, 11:54 AM

swh/lister/cgit/lister.py
108	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
52	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
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 a subscriber: ardumont.Jan 25 2021, 3:00 PM

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.

Harbormaster completed remote builds in B18713: Diff 17578.Jan 25 2021, 3:01 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
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.