Page MenuHomeSoftware Heritage
Differential D3953

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

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

Details

Reviewers
tenma
Group Reviewers
Reviewers
Maniphest Tasks
T1532: Cleanup deprecated configuration code in swh modules
Commits
rDDEP7f5241191352: config: Clarify the configuration setup path for the server part
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

Event Timeline

ardumont created this revision.Sep 15 2020, 1:58 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptSep 15 2020, 1:58 PM
swh-public-ci added a comment.Sep 15 2020, 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

Harbormaster failed remote builds in B15130: Diff 13906!Sep 15 2020, 1:58 PM
ardumont edited the summary of this revision. (Show Details)Sep 15 2020, 3:41 PM
tenma added a subscriber: tenma.Sep 15 2020, 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.Sep 15 2020, 3:57 PM
swh/deposit/config.py
96–97

no more super() things needed

ardumont added inline comments.Sep 15 2020, 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.Sep 15 2020, 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.Sep 15 2020, 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.Sep 15 2020, 4:17 PM
swh/deposit/config.py
96–97

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

ardumont added inline comments.Sep 15 2020, 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.Sep 15 2020, 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)

swh-public-ci added a comment.Sep 15 2020, 5:12 PM

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.

Harbormaster completed remote builds in B15133: Diff 13910.Sep 15 2020, 5:13 PM
ardumont added inline comments.Sep 15 2020, 5:21 PM
swh/deposit/config.py
102

self.scheduler: SchedulerInterface...

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

ardumont updated this revision to Diff 13911.Sep 15 2020, 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.Sep 15 2020, 5:29 PM

Drop unneeded logger instruction

swh-public-ci added a comment.Sep 15 2020, 5:30 PM

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.

Harbormaster completed remote builds in B15134: Diff 13911.Sep 15 2020, 5:30 PM
swh-public-ci added a comment.Sep 15 2020, 5:32 PM

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.

Harbormaster completed remote builds in B15135: Diff 13912.Sep 15 2020, 5:32 PM
ardumont edited the summary of this revision. (Show Details)Sep 15 2020, 5:34 PM
tenma accepted this revision.Sep 15 2020, 6:07 PM
This revision is now accepted and ready to land.Sep 15 2020, 6:07 PM
ardumont updated this revision to Diff 13935.Sep 15 2020, 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.Sep 15 2020, 6:35 PM
ardumont edited the summary of this revision. (Show Details)
swh-public-ci added a comment.Sep 15 2020, 6:38 PM

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.

Harbormaster completed remote builds in B15146: Diff 13935.Sep 15 2020, 6:38 PM
ardumont mentioned this in D3968: conf/deposit: Explicit deposit api configuration.Sep 16 2020, 6:40 PM
ardumont updated this revision to Diff 13990.Sep 16 2020, 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.Sep 16 2020, 6:51 PM
swh-public-ci added a comment.Sep 16 2020, 6:52 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.

Harbormaster completed remote builds in B15202: Diff 13990.Sep 16 2020, 6:52 PM
Closed by commit rDDEP7f5241191352: config: Clarify the configuration setup path for the server part (authored by ardumont). · Explain WhySep 17 2020, 9:22 AM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDDEP7f5241191352: config: Clarify the configuration setup path for the server part.
ardumont mentioned this in D3969: conf/deposit: Explicit deposit api configuration.Sep 17 2020, 12:27 PM
tenma mentioned this in T1410: Kill implicit configuration: new configuration scheme.Sep 17 2020, 5:20 PM
ardumont mentioned this in T1532: Cleanup deprecated configuration code in swh modules.Oct 5 2020, 12:59 PM

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
2 lines--
swh/
deposit/
api/
private/
24 lines98%
Loading...
47 lines93%
Loading...
tests/
api/
18 lines100%
Loading...
64 lines96%
Loading...

Diff 13990

View Options

requirements-swh.txt

Loading...
View Options

swh/deposit/api/private/deposit_read.py

Loading...
View Options

swh/deposit/config.py

Loading...
View Options

swh/deposit/tests/api/test_deposit_schedule.py

Loading...
View Options

swh/deposit/tests/conftest.py

Loading...
About · Developer information · Legal