Page MenuHomeSoftware Heritage

Make stateless lister constructors compatible with credentials
ClosedPublic

Authored by ardumont on Jan 28 2021, 1:34 PM.

Details

Summary

In effect, it just allows to add credentials to cgit, cran and pypi listers.

This fixes instances of error [1]

[1] https://sentry.softwareheritage.org/share/issue/2c35a9f129cf4982a2dd003a232d507a/

Related to T2998

Test Plan

Tested on staging and it worked.

Diff Detail

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

Event Timeline

Build is green

Patch application report for D4961 (id=17702)

Rebasing onto 72be074a79...

Current branch diff-target is up to date.
Changes applied before test
commit b6ec36a44e1c73a55079b157ece9379aef3aa2c4
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Jan 28 13:31:45 2021 +0100

    cgit: Make lister constructor compatible
    
    In effect, it just allows to add credentials even though those are not used. This fixes
    issue [1]
    
    [1] https://sentry.softwareheritage.org/share/issue/2c35a9f129cf4982a2dd003a232d507a/
    
    Related to T2998

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

anlambert added a subscriber: anlambert.

LGTM. Other ported listers might have the same issue so I guess they should be fixed too.

Could you also update the celery tasks tests by mocking credentials configuration coming from configuration file ?

This revision is now accepted and ready to land.Jan 28 2021, 1:45 PM

Could you also update the celery tasks tests by mocking credentials configuration coming from configuration file ?

good idea, i'll check that.


Independently from this diff but still i think possibly related to stateless lister in general, you might be interested by [1]

[1] https://forge.softwareheritage.org/T2998#57500

Could you also update the celery tasks tests by mocking credentials configuration coming from configuration file ?

good idea, i'll check that.


Independently from this diff but still i think possibly related to stateless lister in general, you might be interested by [1]

[1] https://forge.softwareheritage.org/T2998#57500

load_from_envvar = mocker.patch("swh.lister.pattern.load_from_envvar")
load_from_envvar.return_value = {"credentials": {}}

Other ported listers might have the same issue so I guess they should be fixed too.

I had checked the phabricator and gitea one which are already ok btw.
I'll check if i missed others as well.

I'll check if i missed others as well.

yes, pypi and cran needs that patch as well.
I'll amend this diff.

Thanks for the heads up.

load_from_envvar = mocker.patch("swh.lister.pattern.load_from_envvar")
load_from_envvar.return_value = {"credentials": {}}

Well, we can't really do that, we currently totally inhibit the lister.from_configfile call.

load_from_envvar = mocker.patch("swh.lister.pattern.load_from_envvar")
load_from_envvar.return_value = {"credentials": {}}

Well, we can't really do that, we currently totally inhibit the lister.from_configfile call.

Ah right, I guess we should add the test in test_lister.py then. I will update D4962 then.

Well, we can't really do that, we currently totally inhibit the lister.from_configfile call.

Ah right, I guess we should add the test in test_lister.py then. I will update D4962 then.

yes, indeed and that's what i'm currently doing ;)

(great news about launchpad ;)

Well, we can't really do that, we currently totally inhibit the lister.from_configfile call.

Ah right, I guess we should add the test in test_lister.py then. I will update D4962 then.

yes, indeed and that's what i'm currently doing ;)

(great news about launchpad ;)

FYI, I did it like this for launchpad.

def test_lister_from_configfile(swh_scheduler_config, mocker):
    load_from_envvar = mocker.patch("swh.lister.pattern.load_from_envvar")
    load_from_envvar.return_value = {
        "scheduler": {"cls": "local", **swh_scheduler_config},
        "credentials": {},
    }
    lister = LaunchpadLister.from_configfile()
    assert lister.scheduler is not None
    assert lister.credentials is not None

Update cgit/cran/pypi lister to allow credentials to be passed along during instantiation

Build has FAILED

Patch application report for D4961 (id=17705)

Rebasing onto 72be074a79...

Current branch diff-target is up to date.
Changes applied before test
commit f1d69d969f24fa643b3419bb6ef513beff93d3b9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Jan 28 13:31:45 2021 +0100

    cgit: Make lister constructor compatible
    
    In effect, it just allows to add credentials even though those are not used. This fixes
    issue [1]
    
    [1] https://sentry.softwareheritage.org/share/issue/2c35a9f129cf4982a2dd003a232d507a/
    
    Related to T2998

Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/214/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/214/console

Build is green

Patch application report for D4961 (id=17706)

Rebasing onto 72be074a79...

Current branch diff-target is up to date.
Changes applied before test
commit 3e25c6197dc3138534d98b95b6837c076f300caf
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Jan 28 13:31:45 2021 +0100

    cgit: Make lister constructor compatible
    
    In effect, it just allows to add credentials even though those are not used. This fixes
    issue [1]
    
    [1] https://sentry.softwareheritage.org/share/issue/2c35a9f129cf4982a2dd003a232d507a/
    
    Related to T2998

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

Add *Lister.from_configfile test for stateless listers (cgit, cran, pypi)

ardumont retitled this revision from cgit: Make lister constructor compatible to Make stateless lister constructors compatible with credentials.Jan 28 2021, 2:44 PM
ardumont edited the summary of this revision. (Show Details)

Build is green

Patch application report for D4961 (id=17708)

Rebasing onto 72be074a79...

Current branch diff-target is up to date.
Changes applied before test
commit ae17b6b9a04a473eb5957572ea2063f685bcf194
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Jan 28 13:31:45 2021 +0100

    Make stateless lister constructors compatible with credentials
    
    In effect, it just allows to add credentials to cgit, cran and pypi listers.
    
    This fixes instances of error [1]
    
    [1] https://sentry.softwareheritage.org/share/issue/2c35a9f129cf4982a2dd003a232d507a/
    
    Related to T2998

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