Page MenuHomeSoftware Heritage
Differential D4961

Make stateless lister constructors compatible with credentials
ClosedPublic
Actions

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

Details

Reviewers
anlambert
Group Reviewers
Reviewers
Maniphest Tasks
T2998: Deploy lister next-gen in staging
Commits
rDLSae17b6b9a04a: Make stateless lister constructors compatible with credentials
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
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.Jan 28 2021, 1:34 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJan 28 2021, 1:34 PM
swh-public-ci added a comment.Jan 28 2021, 1:38 PM

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.

Harbormaster completed remote builds in B18843: Diff 17702.Jan 28 2021, 1:38 PM
ardumont requested review of this revision.Jan 28 2021, 1:38 PM
anlambert accepted this revision.Jan 28 2021, 1:45 PM
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
ardumont added a comment.Jan 28 2021, 1:48 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

ardumont edited the test plan for this revision. (Show Details)Jan 28 2021, 1:49 PM
anlambert added a comment.Jan 28 2021, 1:50 PM
In D4961#124922, @ardumont wrote:

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": {}}
ardumont added a comment.Jan 28 2021, 1:50 PM

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.

ardumont added a comment.Jan 28 2021, 1:54 PM

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.

ardumont added a comment.Jan 28 2021, 2:08 PM

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.

anlambert added a comment.Jan 28 2021, 2:13 PM
In D4961#124940, @ardumont wrote:

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.

ardumont added a comment.Jan 28 2021, 2:18 PM

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

anlambert added a comment.Jan 28 2021, 2:21 PM
In D4961#124944, @ardumont wrote:

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
ardumont updated this revision to Diff 17705.Jan 28 2021, 2:29 PM

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

ardumont updated this revision to Diff 17706.Jan 28 2021, 2:31 PM

Fix typos

swh-public-ci added a comment.Jan 28 2021, 2:32 PM

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

Harbormaster failed remote builds in B18846: Diff 17705!Jan 28 2021, 2:32 PM
swh-public-ci added a comment.Jan 28 2021, 2:36 PM

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.

Harbormaster completed remote builds in B18847: Diff 17706.Jan 28 2021, 2:36 PM
ardumont updated this revision to Diff 17708.Jan 28 2021, 2:43 PM

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)
swh-public-ci added a comment.Jan 28 2021, 2:47 PM

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.

Harbormaster completed remote builds in B18849: Diff 17708.Jan 28 2021, 2:47 PM
Closed by commit rDLSae17b6b9a04a: Make stateless lister constructors compatible with credentials (authored by ardumont). · Explain WhyJan 28 2021, 2:50 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLSae17b6b9a04a: Make stateless lister constructors compatible with credentials.

Revision Contents
Changeset List

PathSize
swh/
lister/
cgit/
10 lines
tests/
36 lines
6 lines
cran/
10 lines
tests/
31 lines
pypi/
14 lines
tests/
31 lines

Diff 17709

View Options

swh/lister/cgit/lister.py

Loading...
View Options

swh/lister/cgit/tests/test_lister.py

Loading...
View Options

swh/lister/cgit/tests/test_tasks.py

Loading...
View Options

swh/lister/cran/lister.py

Loading...
View Options

swh/lister/cran/tests/test_lister.py

Loading...
View Options

swh/lister/pypi/lister.py

Loading...
View Options

swh/lister/pypi/tests/test_lister.py

Loading...
About · Developer information · Legal