Page MenuHomeSoftware Heritage

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

Authored by ardumont on Sep 15 2020, 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
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15130
Build 23325: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 23324: arc lint + arc unit

Event Timeline

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

tenma added inline comments.
swh/deposit/config.py
100

config is not optional be cause at least DEFAULT_CONFIG always exists

swh/deposit/config.py
104

no more super() things needed

swh/deposit/config.py
100

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

104

yeah, i'll check.

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

swh/deposit/config.py
100

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

104

I meant, for this class.

swh/deposit/config.py
100

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)

swh/deposit/config.py
100

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

swh/deposit/config.py
100

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.

  • 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.

swh/deposit/config.py
110

self.scheduler: SchedulerInterface...

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

  • 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!

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.

This revision is now accepted and ready to land.Sep 15 2020, 6:07 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.Sep 15 2020, 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 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.Sep 16 2020, 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.