Page MenuHomeSoftware Heritage
Differential D4954

cgit: Don't stop the listing when a repository page is not available
ClosedPublic
Actions

Authored by vsellier on Jan 27 2021, 12:42 PM.

Details

Reviewers
anlambert
Group Reviewers
Reviewers
Maniphest Tasks
T2988: Improve cgit lister to add last modification date of the repos
Commits
rDLSf6f9f1ca28a9: cgit: Don't stop the listing when a repository page is not available
Summary

Related to T2988

Diff Detail

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

Event Timeline

vsellier created this revision.Jan 27 2021, 12:42 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJan 27 2021, 12:42 PM
swh-public-ci added a comment.Jan 27 2021, 12:47 PM

Build is green

Patch application report for D4954 (id=17655)

Could not rebase; Attempt merge onto bb0184c004...

Updating bb0184c..81b45e3
Fast-forward
 swh/lister/cgit/lister.py            |  57 ++++++++++++++++---
 swh/lister/cgit/tests/test_lister.py | 104 +++++++++++++++++++++++++++++++++--
 2 files changed, 148 insertions(+), 13 deletions(-)
Changes applied before test
commit 81b45e3c15b11f74efc8c05e6af46a2bd72e1831
Author: Vincent SELLIER <vincent.sellier@softwareheritage.org>
Date:   Wed Jan 27 12:22:47 2021 +0100

    cgit: Don't stop the listing when a repository page is not available
    
    Related to T2988

commit f1602ab87217f105e3245dc5058c281cb39a1b82
Author: Vincent SELLIER <vincent.sellier@softwareheritage.org>
Date:   Tue Jan 26 18:03:27 2021 +0100

    cgit: Add support for last_update information during listing
    
    Related to T2988

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

Harbormaster completed remote builds in B18794: Diff 17655.Jan 27 2021, 12:47 PM
vsellier requested review of this revision.Jan 27 2021, 12:47 PM
vsellier updated this revision to Diff 17668.Jan 27 2021, 2:04 PM

rebase

swh-public-ci added a comment.Jan 27 2021, 2:10 PM

Build is green

Patch application report for D4954 (id=17668)

Could not rebase; Attempt merge onto bb0184c004...

Updating bb0184c..c2d4d30
Fast-forward
 swh/lister/cgit/lister.py            |  53 +++++++++++++++---
 swh/lister/cgit/tests/test_lister.py | 104 +++++++++++++++++++++++++++++++++--
 2 files changed, 144 insertions(+), 13 deletions(-)
Changes applied before test
commit c2d4d30e8af83b91a289837e2daaab55bb4244ab
Author: Vincent SELLIER <vincent.sellier@softwareheritage.org>
Date:   Wed Jan 27 12:22:47 2021 +0100

    cgit: Don't stop the listing when a repository page is not available
    
    Related to T2988

commit 743bff42190060c7522b8d00ead1f2bfcd10f5f6
Author: Vincent SELLIER <vincent.sellier@softwareheritage.org>
Date:   Tue Jan 26 18:03:27 2021 +0100

    cgit: Add support for last_update information during listing
    
    Related to T2988

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

Harbormaster completed remote builds in B18807: Diff 17668.Jan 27 2021, 2:10 PM
vsellier updated this revision to Diff 17670.Jan 27 2021, 2:19 PM

rebase

anlambert requested changes to this revision.Jan 27 2021, 2:24 PM
anlambert added a subscriber: anlambert.

You should rather catch the requests.exceptions.HTTPError originally raised by _get_and_parse method.

Simply modifying the get_origins_from_page method like this should do the trick.

def get_origins_from_page(
    self, repositories: Repositories
) -> Iterator[ListedOrigin]:
    """Convert a page of cgit repositories into a list of ListedOrigins."""
    assert self.lister_obj.id is not None

    for repository in repositories:
        try:
            origin_url = self._get_origin_from_repository_url(repository["url"])
        except HTTPError as e:
            logger.warning(
                "Unexpected HTTP status code %s on %s",
                e.response.status_code,
                e.response.url,
            )
            origin_url = None

        if not origin_url:
            continue

        yield ListedOrigin(
            lister_id=self.lister_obj.id,
            url=origin_url,
            visit_type="git",
            last_update=_parse_last_updated_date(repository),
        )
This revision now requires changes to proceed.Jan 27 2021, 2:24 PM
swh-public-ci added a comment.Jan 27 2021, 2:26 PM

Build is green

Patch application report for D4954 (id=17670)

Could not rebase; Attempt merge onto bb0184c004...

Updating bb0184c..aa7688f
Fast-forward
 swh/lister/cgit/lister.py            |  58 ++++++++++++++++---
 swh/lister/cgit/tests/test_lister.py | 104 +++++++++++++++++++++++++++++++++--
 2 files changed, 149 insertions(+), 13 deletions(-)
Changes applied before test
commit aa7688f5be5c9efbe78dc2419e884696736d1dcf
Author: Vincent SELLIER <vincent.sellier@softwareheritage.org>
Date:   Wed Jan 27 12:22:47 2021 +0100

    cgit: Don't stop the listing when a repository page is not available
    
    Related to T2988

commit 91fcde83410d41daf0dbe4f431b55c33c69f8544
Author: Vincent SELLIER <vincent.sellier@softwareheritage.org>
Date:   Tue Jan 26 18:03:27 2021 +0100

    cgit: Add support for last_update information during listing
    
    Related to T2988

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

Harbormaster completed remote builds in B18809: Diff 17670.Jan 27 2021, 2:26 PM
ardumont added a subscriber: ardumont.EditedJan 27 2021, 2:27 PM

You should rather catch the requests.exceptions.HTTPError originally raised by _get_and_parse method.

picture me curious, why?

(it's missing the eventual logging.warning instruction but its use is debatable as well ;)

anlambert added a comment.EditedJan 27 2021, 2:38 PM
In D4954#124697, @ardumont wrote:

You should rather catch the requests.exceptions.HTTPError originally raised by _get_and_parse method.

picture me curious, why?

(it's missing the eventual logging.warning instruction but its use is debatable as well ;)

Less code to modify, easier to read and understand the intended behavior from my point of view.

ardumont added a comment.Jan 27 2021, 2:46 PM

Less code to modify and easier to read and understand the intended behavior from my point of view.

maybe so.

One pro I see for your implementation is if at some point the main loop (get_pages)
fails to fetch something (which we don't currently expect to see the assert added
here), we'll have more information available in the log instead of an AssertionError.
We'll indeed have an actual HttpError with the status code and everything in the
stacktrace.

All in all, i think you are right ;)

vsellier updated this revision to Diff 17672.Jan 27 2021, 2:54 PM

Use an exception to validate a repo page can be accessed

anlambert added a comment.Jan 27 2021, 2:54 PM
In D4954#124699, @ardumont wrote:

Less code to modify and easier to read and understand the intended behavior from my point of view.

maybe so.

One pro I see for your implementation is if at some point the main loop (get_pages)
fails to fetch something (which we don't currently expect to see the assert added
here), we'll have more information available in the log instead of an AssertionError.
We'll indeed have an actual HttpError with the status code and everything in the
stacktrace.

All in all, i think you are right ;)

Yes, it's better to have a method that simply performs an HTTP request and raise if errors.
HTTP errors can then be handled if needed in other methods and if not the lister
will end up with error with the stack trace displayed.

swh-public-ci added a comment.Jan 27 2021, 2:57 PM

Build is green

Patch application report for D4954 (id=17672)

Rebasing onto 91fcde8341...

Current branch diff-target is up to date.
Changes applied before test
commit f6f9f1ca28a9161a2c3606243e202285030f7db6
Author: Vincent SELLIER <vincent.sellier@softwareheritage.org>
Date:   Wed Jan 27 12:22:47 2021 +0100

    cgit: Don't stop the listing when a repository page is not available
    
    Related to T2988

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

Harbormaster completed remote builds in B18811: Diff 17672.Jan 27 2021, 2:57 PM
anlambert accepted this revision.Jan 27 2021, 3:03 PM

Looks good !

This revision is now accepted and ready to land.Jan 27 2021, 3:03 PM
vsellier added a comment.Jan 27 2021, 3:05 PM

Thanks :)

Closed by commit rDLSf6f9f1ca28a9: cgit: Don't stop the listing when a repository page is not available (authored by vsellier). · Explain WhyJan 27 2021, 3:06 PM
This revision was automatically updated to reflect the committed changes.
vsellier added a commit: rDLSf6f9f1ca28a9: cgit: Don't stop the listing when a repository page is not available.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
lister/
cgit/
7 lines97%
Loading...
tests/
18 lines100%
Loading...

Diff 17668

View Options

swh/lister/cgit/lister.py

Loading...
View Options

swh/lister/cgit/tests/test_lister.py

Loading...
About · Developer information · Legal