Page MenuHomeSoftware Heritage

config: Clarify the configuration setup path for the server part
ClosedPublic

Authored by ardumont on Tue, Sep 15, 1:58 PM.

Details

Summary

Mixin SWHConfig is deprecated. This unplugs the inheritance to it (too much
complexity for no good reason).

Feel free to propose something better. My plan is to currently unplug this
everywhere where it's left and then remove that SWHConfig mixin [1]

[1] T1532#48568

Related to T1532

Test Plan

tox

Diff Detail

Repository
rDDEP Push deposit
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.Tue, Sep 15, 1:58 PM

Build has FAILED

Patch application report for D3953 (id=13906)

Rebasing onto f41a5a6c06...

First, rewinding head to replay your work on top of it...
Applying: config: Clarify configuration setup
Changes applied before test
commit 8bdec9a5dcc3a7bbe2fc63c167687e1ea2b6d9f7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 15 10:51:37 2020 +0200

    config: Clarify configuration setup
    
    Mixin SWHConfig is deprecated. This unplugs the inheritance to that class which
    has becomes too much complicated to follow.
    
    Related to T1532

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

ardumont edited the summary of this revision. (Show Details)Tue, Sep 15, 3:41 PM
tenma added a subscriber: tenma.Tue, Sep 15, 3:56 PM
tenma added inline comments.
swh/deposit/config.py
96–97

config is not optional be cause at least DEFAULT_CONFIG always exists

tenma added inline comments.Tue, Sep 15, 3:57 PM
swh/deposit/config.py
96–97

no more super() things needed

ardumont added inline comments.Tue, Sep 15, 3:58 PM
swh/deposit/config.py
96–97

yeah, but it is at that moment i declare it.
If i remove it, mypy will complain.

96–97

yeah, i'll check.

Not really sure because in the deposit, class also inherits from other classes.

tenma added inline comments.Tue, Sep 15, 4:01 PM
swh/deposit/config.py
96–97

I meant, for this class.

96–97

really? There should be a way because every attribute would need optional that way, there must be a standard way

tenma added inline comments.Tue, Sep 15, 4:12 PM
swh/deposit/config.py
96–97

did you try to drop the null initializations? that should do the trick. You're not required to initialise when you declare, and it is not recommended for instance variable (vs. class variables)

ardumont added inline comments.Tue, Sep 15, 4:17 PM
swh/deposit/config.py
96–97

This sounds sensible, I'll do that ;)
Thanks.

ardumont added inline comments.Tue, Sep 15, 4:19 PM
swh/deposit/config.py
96–97

well, something in the test must change as well then because that's not happy about it then:

AttributeError: <class 'swh.deposit.config.APIConfig'> has no attribute 'config'

i'll check further soon.

ardumont updated this revision to Diff 13910.Tue, Sep 15, 5:10 PM
  • Namespace deposit configuration
  • Reduce monkeypatch stanza
  • fix build by requiring swh.core[http] deps

(I may have an improvment to avoid monkeypatch further)

Build is green

Patch application report for D3953 (id=13910)

Rebasing onto f41a5a6c06...

First, rewinding head to replay your work on top of it...
Applying: config: Clarify configuration setup
Changes applied before test
commit 691607134d6e46e9a137c5759e2f1e821ac053a3
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 15 10:51:37 2020 +0200

    config: Clarify configuration setup
    
    Mixin SWHConfig is deprecated. This unplugs the inheritance to that class which
    has becomes too much complicated to follow.
    
    Related to T1532

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

ardumont added inline comments.Tue, Sep 15, 5:21 PM
swh/deposit/config.py
103

self.scheduler: SchedulerInterface...

exactly why i want to stop that monkeypatch thing...

ardumont updated this revision to Diff 13911.Tue, Sep 15, 5:27 PM
  • Drop namespacing, this is out of scope for this diff
  • Make sure the APIConfig code is used during testing consistently with the rest
  • Fix type typo

tests should be green!

ardumont updated this revision to Diff 13912.Tue, Sep 15, 5:29 PM

Drop unneeded logger instruction

Build is green

Patch application report for D3953 (id=13911)

Rebasing onto f41a5a6c06...

First, rewinding head to replay your work on top of it...
Applying: config: Clarify configuration setup
Changes applied before test
commit c3056bc8ec74d463ceb635aef570fd86395165e9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 15 10:51:37 2020 +0200

    config: Clarify configuration setup
    
    Mixin SWHConfig is deprecated. This unplugs the inheritance to that class which
    has becomes too much complicated to follow.
    
    Related to T1532

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

Build is green

Patch application report for D3953 (id=13912)

Rebasing onto f41a5a6c06...

First, rewinding head to replay your work on top of it...
Applying: config: Clarify configuration setup
Changes applied before test
commit 0f1a6d949744b982432388f84006fad37a965bfa
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 15 10:51:37 2020 +0200

    config: Clarify configuration setup
    
    Mixin SWHConfig is deprecated. This unplugs the inheritance to that class which
    has becomes too much complicated to follow.
    
    Related to T1532

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

ardumont edited the summary of this revision. (Show Details)Tue, Sep 15, 5:34 PM
tenma accepted this revision.Tue, Sep 15, 6:07 PM
This revision is now accepted and ready to land.Tue, Sep 15, 6:07 PM
ardumont updated this revision to Diff 13935.Tue, Sep 15, 6:35 PM
ardumont edited the summary of this revision. (Show Details)
  • Rebase on latest master
  • rework commit message
ardumont retitled this revision from config: Clarify configuration setup to config: Clarify configuration setup server code.Tue, Sep 15, 6:35 PM
ardumont edited the summary of this revision. (Show Details)

Build is green

Patch application report for D3953 (id=13935)

Rebasing onto f41a5a6c06...

Current branch diff-target is up to date.
Changes applied before test
commit fbd84db9424b8f8b2ea102c5f84ef04b78d90b8c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 15 10:51:37 2020 +0200

    config: Clarify configuration setup for server code
    
    Mixin SWHConfig is deprecated. This unplugs the inheritance to that class which
    has becomes too much complicated to follow.
    
    Related to T1532

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

ardumont updated this revision to Diff 13990.Wed, Sep 16, 6:50 PM
ardumont edited the summary of this revision. (Show Details)

Rework commit message and add missing cleanup part.

ardumont retitled this revision from config: Clarify configuration setup server code to config: Clarify the configuration setup path for the server part.Wed, Sep 16, 6:51 PM

Build is green

Patch application report for D3953 (id=13990)

Rebasing onto f41a5a6c06...

Current branch diff-target is up to date.
Changes applied before test
commit 7f5241191352fcb170ced98bed745827cde63cbd
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 15 10:51:37 2020 +0200

    config: Clarify the configuration setup path for the server part
    
    Mixin SWHConfig is deprecated.
    
    Related to T1532

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