lister: Make sure lister that requires github tokens can use it
ClosedPublic
Actions

Authored by ardumont on Oct 26 2022, 5:29 PM.

Details

Reviewers

anlambert

Group Reviewers

Reviewers

Commits

rDLS92d494261f26: lister: Make sure lister that requires github tokens can use it

Summary

Deploying the nixguix lister, I realized that even though the credentials configuration
is properly set for all listers, the listers actually requiring github origin
canonicalization do not have access to the github credentials. It's lost during the
constructor to only focus on the lister's credentials. Which currently translates to
listers being rate-limited (when doing the github canonicalization).

This commit fixes it by pushing the self.github_session instantiation in the constructor
when the lister explicitely requires the github session. Hence lifting the rate limit
for maven, packagist, nixguix, and github listers.

Related to infra/sysadm-environment#4655

Test Plan

Tests should still be fine

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

ardumont created this revision.Oct 26 2022, 5:29 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptOct 26 2022, 5:29 PM

Build is green

Patch application report for D8783 (id=31651)

Rebasing onto 81688ca17e...

Current branch diff-target is up to date.

Changes applied before test

commit 92d494261f267d6552d9a24361fd612974f1c982
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Oct 26 17:23:40 2022 +0200

    lister: Make sure lister that requires github tokens can use it
    
    Deploying the nixguix lister, I realized that even though the credentials configuration
    is properly set for all listers, the listers actually requiring github origin
    canonicalization do not have access to the github credentials. It's lost during the
    constructor to only focus on the lister's credentials. Which currently translates to
    listers being rate-limited.
    
    This commit fixes it by pushing the self.github_session instantiation in the constructor
    when the lister explicitely requires the github session. Hence lifting the rate limit
    for maven, packagist, nixguix, and github listers.
    
    Related to infra/sysadm-environment#4655

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

Better like this indeed, could you add a test with a sample credentials config file to check github_session object has loaded credentials ?

Harbormaster completed remote builds in B32604: Diff 31651.Oct 26 2022, 5:35 PM

ardumont requested review of this revision.Oct 26 2022, 5:35 PM

ardumont edited the summary of this revision. (Show Details)Oct 26 2022, 5:36 PM

In D8783#228307, @anlambert wrote:

Better like this indeed, could you add a test with a sample credentials config file to check github_session object has loaded credentials ?

Nevermind, there is already some in github lister tests.

This revision is now accepted and ready to land.Oct 26 2022, 5:37 PM

ardumont edited the test plan for this revision. (Show Details)Oct 26 2022, 5:37 PM

Closed by commit rDLS92d494261f26: lister: Make sure lister that requires github tokens can use it (authored by ardumont). · Explain WhyOct 26 2022, 5:38 PM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDLS92d494261f26: lister: Make sure lister that requires github tokens can use it.