Page MenuHomeSoftware Heritage

cgit: rewrite the CGit lister
ClosedPublic

Authored by douardda on Fri, Aug 30, 6:03 PM.

Details

Summary

Simplify the code:

  • do only inherit from ListerBase
  • implement HTTP queries directly using requests
  • get rid of convoluted code

Make the origin_url gathered from the git repo's "project" page instead of
building it from the 'url_prefix' hack. Now, the lister WILL make substancially
more requests, since it will make one request per listed git repo, but
the provided origin_url should be pretty reliable now.

When several url are provided as clonable URLs, choose the http/https one first,
otherwise, choose the first one of the list.

Add proper tests for the cgit lister.

Also, get rid of the 'time_updated' column in the model.

Depends on D1928

Diff Detail

Repository
rDLS Listers
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

douardda created this revision.Fri, Aug 30, 6:03 PM

This one, i'll have a look on monday ;)

Don't this need a rebase (to make jenkins happy)?
The jenkins failure seems to be out of its depth.

I just have a minor nitpick about the tests with multiple pages.

Otherwise, this looks good, thx.

swh/lister/cgit/lister.py
132

I guess the html_url, full_name, etc... and other unpopulated field dbs are defaulting to null values (so it does not break ;).
And are also what needs to be further dealt with via T1978

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

Don't we want to check a tad the urls as you did in the other tests?

Don't this need a rebase (to make jenkins happy)?
The jenkins failure seems to be out of its depth.

I thinks this needs a fix in cli.py (which is undone/deleted in D1504 but meh). Let me check that.

douardda added inline comments.Mon, Sep 2, 10:58 AM
swh/lister/cgit/lister.py
132

That's the idea yes.

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

As you wish, master ;-)

ardumont added a comment.EditedMon, Sep 2, 11:06 AM

Btw, i think we can make this related to T1861 (bullet 1. as implementation).

nahimilega added inline comments.
swh/lister/cgit/lister.py
13–14

You could add a docstring to the class. Something like this https://forge.softwareheritage.org/source/swh-lister/browse/master/swh/lister/packagist/lister.py$0-15
IIRC, we decided to make a docstring for the lister class which shows their output.
I forgot to create a task regarding this(my bad)

nahimilega added inline comments.Mon, Sep 2, 11:17 AM
swh/lister/cgit/lister.py
112–131

This line is the same as line 57. Maybe we could make a function for this.

ardumont added inline comments.Mon, Sep 2, 11:54 AM
swh/lister/cgit/lister.py
13–14

Either the class or the init as whatever is more suited for such documentation.

douardda updated this revision to Diff 6499.Mon, Sep 2, 12:13 PM

Add some docstring and ensure tests pass ok

ardumont added inline comments.Mon, Sep 2, 12:17 PM
swh/lister/cgit/lister.py
25

to be used

ardumont accepted this revision.Mon, Sep 2, 12:19 PM
This revision is now accepted and ready to land.Mon, Sep 2, 12:19 PM
douardda added inline comments.Mon, Sep 2, 12:20 PM
swh/lister/cgit/lister.py
25

seen that, and also 'gather published "Clone" URLs' (without 'the')

douardda updated this revision to Diff 6505.Mon, Sep 2, 12:30 PM

typo in the docstring

This revision was automatically updated to reflect the committed changes.