Page MenuHomeSoftware Heritage

config: Deprecate SWHConfig mixin in favor of the load_from_envvar function
Changes PlannedPublic

Authored by ardumont on Wed, Sep 16, 3:22 PM.

Details

Summary

That new function declares exactly what happens today during our configuration
loading (for loader/lister/indexer/... and rpc services in general not for the
CLIs which will be dealt with later).

Loads and parses the yaml configuration file out of the SWH_CONFIG_FILENAME
environment variable. Allowing eventual dict enrichment with default
configuration if provided.

Everything docker/staging/production related is setting the SWH_CONFIG_FILENAME in the environment [1-5]
(including the tests now as well).

This currently impacts (which I will deal with soon):

$SWH_ENVIRONMENT_HOME/swh-indexer/swh/indexer/indexer.py:from swh.core.config import SWHConfig
$SWH_ENVIRONMENT_HOME/swh-indexer/swh/indexer/indexer.py:class BaseIndexer(SWHConfig, metaclass=abc.ABCMeta):
$SWH_ENVIRONMENT_HOME/swh-indexer/swh/indexer/rehash.py:from swh.core.config import SWHConfig
$SWH_ENVIRONMENT_HOME/swh-indexer/swh/indexer/rehash.py:class RecomputeChecksums(SWHConfig):
$SWH_ENVIRONMENT_HOME/swh-lister/swh/lister/core/lister_base.py:class ListerBase(abc.ABC, config.SWHConfig):
$SWH_ENVIRONMENT_HOME/swh-loader-core/swh/loader/core/loader.py:class BaseLoader(config.SWHConfig, metaclass=ABCMeta):
$SWH_ENVIRONMENT_HOME/swh-loader-core/swh/loader/package/loader.py:from swh.core.config import SWHConfig
$SWH_ENVIRONMENT_HOME/swh-loader-core/swh/loader/package/loader.py:        self.config = SWHConfig.parse_config_file()

[1] prod/staging: https://forge.softwareheritage.org/source/puppet-swh-site/browse/production/?grep=SWH_CONFIG_FILENAME
[2] docker: https://forge.softwareheritage.org/source/swh-environment/browse/master/?grep=SWH_CONFIG_FILENAME

As sample:
[3] loader/lister: https://forge.softwareheritage.org/source/puppet-swh-site/browse/production/site-modules/profile/templates/swh/deploy/worker/swh-worker@.service.erb$10
[4] scheduler: https://forge.softwareheritage.org/source/swh-environment/browse/master/docker/docker-compose.yml$109
[5] api/rpc servers: https://forge.softwareheritage.org/source/puppet-swh-site/browse/production/site-modules/profile/manifests/swh/deploy/rpc_server.pp$104

Related to T1532
Related to T1410

Test Plan

tox

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Build is green

Patch application report for D3965 (id=13957)

Could not rebase; Attempt merge onto 60c535ddee...

Updating 60c535d..5c05fa7
Fast-forward
 swh/core/config.py            | 135 ++++++++++++++----------------------------
 swh/core/tests/test_config.py |  97 +++++++++++++++++++++++-------
 2 files changed, 122 insertions(+), 110 deletions(-)
Changes applied before test
commit 5c05fa7170486abeb1f9e23a4f076e34db75a98c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:23:16 2020 +0200

    swh.core.config: Simplify SWHConfig mixin to the actual use
    
    Related to T1532

commit 66ac6ed3aa31a63da3226f2857755f7398721d31
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 14:32:19 2020 +0200

    swh.core.config: Drop support for ini configuration file
    
    This is no longer used.
    
    This also adds some types on impacted changed functions.
    
    Related to T1532

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

ardumont updated this revision to Diff 13958.Wed, Sep 16, 3:28 PM

Drop unnecessary typing on un-impacted functions (i'll open another diff for it)

Build is green

Patch application report for D3965 (id=13958)

Could not rebase; Attempt merge onto 60c535ddee...

Updating 60c535d..29414b5
Fast-forward
 swh/core/config.py            | 119 +++++++++++++-----------------------------
 swh/core/tests/test_config.py |  97 ++++++++++++++++++++++++++--------
 2 files changed, 113 insertions(+), 103 deletions(-)
Changes applied before test
commit 29414b50cc6f009f7413792d0658c47eaca04a1e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:25:52 2020 +0200

    swh.core.config: Simplify SWHConfig mixin to the actual use
    
    Related to T1532

commit 66ac6ed3aa31a63da3226f2857755f7398721d31
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 14:32:19 2020 +0200

    swh.core.config: Drop support for ini configuration file
    
    This is no longer used.
    
    This also adds some types on impacted changed functions.
    
    Related to T1532

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

ardumont updated this revision to Diff 13960.Wed, Sep 16, 3:40 PM

Drop the typed DEFAULT_CONFIG and use a simple DEFAULT_CONFIG as dict

Build is green

Patch application report for D3965 (id=13960)

Could not rebase; Attempt merge onto 60c535ddee...

Updating 60c535d..7e3ed44
Fast-forward
 swh/core/config.py            | 122 ++++++++++++++----------------------------
 swh/core/tests/test_config.py |  97 +++++++++++++++++++++++++--------
 2 files changed, 116 insertions(+), 103 deletions(-)
Changes applied before test
commit 7e3ed44614cf5978fc57f3b5cd6894ab6d3212e7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:25:52 2020 +0200

    swh.core.config: Simplify SWHConfig mixin to the actual use
    
    Related to T1532

commit 66ac6ed3aa31a63da3226f2857755f7398721d31
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 14:32:19 2020 +0200

    swh.core.config: Drop support for ini configuration file
    
    This is no longer used.
    
    This also adds some types on impacted changed functions.
    
    Related to T1532

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

ardumont updated this revision to Diff 13963.Wed, Sep 16, 3:54 PM

Fix DEFAULT_CONFIG type

Build is green

Patch application report for D3965 (id=13963)

Could not rebase; Attempt merge onto 60c535ddee...

Updating 60c535d..e7b3048
Fast-forward
 swh/core/config.py            | 122 ++++++++++++++----------------------------
 swh/core/tests/test_config.py |  97 +++++++++++++++++++++++++--------
 2 files changed, 116 insertions(+), 103 deletions(-)
Changes applied before test
commit e7b3048844622fe74b83c54682aaa464e34f09b1
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:25:52 2020 +0200

    swh.core.config: Simplify SWHConfig mixin to the actual use
    
    Related to T1532

commit 66ac6ed3aa31a63da3226f2857755f7398721d31
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 14:32:19 2020 +0200

    swh.core.config: Drop support for ini configuration file
    
    This is no longer used.
    
    This also adds some types on impacted changed functions.
    
    Related to T1532

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

ardumont retitled this revision from swh.core.config: Simplify SWHConfig mixin to the actual use to swh.core.config: Simplify SWHConfig mixin to its actual use.Wed, Sep 16, 4:24 PM
vlorentz accepted this revision.Wed, Sep 16, 4:24 PM
vlorentz added a subscriber: vlorentz.
vlorentz added inline comments.
swh/core/config.py
378

s/should be defined/is undefined

This revision is now accepted and ready to land.Wed, Sep 16, 4:24 PM
ardumont updated this revision to Diff 13967.Wed, Sep 16, 4:32 PM
  • Adapt value error message according to suggestion
  • Rework docstring to use func:stanza sphinx references
  • Rework commit message

Build is green

Patch application report for D3965 (id=13967)

Rebasing onto 66ac6ed3aa...

Current branch diff-target is up to date.
Changes applied before test
commit 2934097d81da51a6f9b9423bf710c9a9f3116ae2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:25:52 2020 +0200

    swh.core.config: Simplify SWHConfig mixin to its actual use
    
    Related to T1532

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

tenma added a subscriber: tenma.Wed, Sep 16, 4:36 PM
tenma added inline comments.
swh/core/config.py
327
WARNING: mutable value in class definition. As with function definitions, this will only be eval'd once. I think this class is meant to be instanciated, we don't want to share config between instances. In that case, use factory like attrs ones or write a constructor. For this, we better think of a strategy that does not involves user discipline but works automatically.
380

A default config, albeit empty, is defined (as dict), and merge_config has no input domain restriction. So this conditional is not needed.

ardumont edited the summary of this revision. (Show Details)Wed, Sep 16, 4:36 PM
ardumont edited the summary of this revision. (Show Details)Wed, Sep 16, 4:56 PM
vlorentz added inline comments.Wed, Sep 16, 5:01 PM
swh/core/config.py
327

alternatively, we can just make it None

ardumont updated this revision to Diff 13983.Wed, Sep 16, 5:07 PM

Change commit order

Build is green

Patch application report for D3965 (id=13983)

Could not rebase; Attempt merge onto 66ac6ed3aa...

Updating 66ac6ed..cf4f7e5
Fast-forward
 swh/core/config.py            | 92 +++++++++++++++----------------------------
 swh/core/tests/test_config.py | 41 +++++++++++++++++++
 2 files changed, 73 insertions(+), 60 deletions(-)
Changes applied before test
commit cf4f7e5d942d83c05f505012a6695e94f796ee1e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:25:52 2020 +0200

    swh.core.config: Simplify SWHConfig mixin to its actual use
    
    Related to T1532

commit 22e77212f1f7704a9d138da12b75304269b1884c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:26:25 2020 +0200

    swh.core.config: Add types

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

tenma added inline comments.Wed, Sep 16, 5:18 PM
swh/core/config.py
327

No, often a bad idea to have nullable types where it is not necessary, for reliability reasons. In this mixin, only a declaration, which conveniently serves as a spec btw, and in user class only an initialization is needed. In fact it could be seen as a lazy constant, which is very safe (reliable) construct.

Build is green

Patch application report for D3965 (id=14025)

Rebasing onto 28d61c8cf1...

Current branch diff-target is up to date.
Changes applied before test
commit 4658518084d4bbe769afcdcf87290f03319a5fe9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:25:52 2020 +0200

    swh.core.config: Simplify SWHConfig mixin to its actual use
    
    Related to T1532

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

tenma added a comment.Mon, Sep 21, 2:48 PM

I think you should not remove this code as it has one use case (filename is provided as parameter, from CLI or not) but have a different function name for the "prod" use case (e.g. parse_config_file_envvar).
Some parts in the old code are not needed anymore, but not all. In another diff we will cover the other use case.
For now better to have 2 functions.

swh/core/config.py
380

I repeat, this branch will always match, so no need to keep it.

I think you should not remove this code as it has one use case (filename is provided as parameter, from CLI or not)
but have a different function name for the "prod" use case (e.g. parse_config_file_envvar).

I am not sure I completely follow. It's not used right now, better remove it so maintenance is clearer.

Some parts in the old code are not needed anymore, but not all.

Currently, it is needed by rpc services, not the CLIs.
Those are using functions from swh.core.config but not the mixin iirc.

In another diff we will cover the other use case.

Yes, in another diff, when the specification you are working on is clear on what it is we must do.
In the mean time, better align the code with production use.

Otherwise, that's still code we have to follow through even though it's not used...

For now better to have 2 functions.

Can you please develop that part?

Thanks for the feedback.
Cheers,

swh/core/config.py
327

WARNING
...

I'm opened to an example of what's implied by that suggestion.

Please, for example, wrap the snippet in tiple backquote

<sample-code>

so it's nicely rendered here ;)

380

yes, I did not adapt it yet since i'm unclear as to what i need to do for the DEFAULT_CONFIG field remark first...

ardumont updated this revision to Diff 14112.Tue, Sep 22, 10:04 AM
  • Drop redundant conditional
  • Rebase on latest master
ardumont planned changes to this revision.Tue, Sep 22, 10:04 AM
ardumont marked 3 inline comments as done.

Build is green

Patch application report for D3965 (id=14112)

Rebasing onto 56d505f34a...

Current branch diff-target is up to date.
Changes applied before test
commit e48d8b59633205b5af760a0169450e22fd654cd1
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:25:52 2020 +0200

    swh.core.config: Simplify SWHConfig mixin to its actual use
    
    Related to T1532

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

ardumont requested review of this revision.Tue, Sep 22, 10:05 AM
This revision is now accepted and ready to land.Tue, Sep 22, 10:05 AM
ardumont planned changes to this revision.Tue, Sep 22, 11:05 AM

After discussing with @tenma, I'll simplify this to a function and finally get rid of that mixin.

ardumont updated this revision to Diff 14117.Tue, Sep 22, 11:13 AM

Drop SWHMixin in favor of new function load_from_envvar

This revision is now accepted and ready to land.Tue, Sep 22, 11:13 AM
ardumont retitled this revision from swh.core.config: Simplify SWHConfig mixin to its actual use to config: Drop SWHConfig mixin in favor of the load_from_envvar function.Tue, Sep 22, 11:14 AM
ardumont edited the summary of this revision. (Show Details)

Build is green

Patch application report for D3965 (id=14117)

Rebasing onto 56d505f34a...

Current branch diff-target is up to date.
Changes applied before test
commit c4ee5db89f0f1490813741937f7fe3dd1e35be30
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:25:52 2020 +0200

    config: Drop SWHConfig mixin in favor of the load_from_envvar function
    
    That new function declares exactly what happens today during our configuration
    loading use. Loads and parses the yaml configuration file out of the
    SWH_CONFIG_FILENAME environment variable. Allowing eventual dict enrichment
    with default configuration if provided.
    
    Related to T1532

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

ardumont edited the summary of this revision. (Show Details)Tue, Sep 22, 11:15 AM
ardumont marked 3 inline comments as done.
ardumont edited the summary of this revision. (Show Details)Tue, Sep 22, 11:18 AM
tenma added inline comments.Tue, Sep 22, 11:30 AM
swh/core/config.py
326

assert "SWH_CONFIG_FILENAME" in os.environ, "SWH_CONFIG_FILENAME environment variable is undefined." est encore plus clair, ça dit "vous avez pas suivi le contrat"

ardumont updated this revision to Diff 14118.Tue, Sep 22, 11:30 AM
ardumont edited the summary of this revision. (Show Details)

Avoid empty dict and use None instead

Build is green

Patch application report for D3965 (id=14118)

Rebasing onto 56d505f34a...

Current branch diff-target is up to date.
Changes applied before test
commit d9d4ef0f742ac40e8e01a8cfe89605a87381a264
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:25:52 2020 +0200

    config: Drop SWHConfig mixin in favor of the load_from_envvar function
    
    That new function declares exactly what happens today during our configuration
    loading use. Loads and parses the yaml configuration file out of the
    SWH_CONFIG_FILENAME environment variable. Allowing eventual dict enrichment
    with default configuration if provided.
    
    Related to T1532

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

ardumont updated this revision to Diff 14119.Tue, Sep 22, 11:33 AM

Adapt according to review

Build is green

Patch application report for D3965 (id=14119)

Rebasing onto 56d505f34a...

Current branch diff-target is up to date.
Changes applied before test
commit 1e90d17646d1067e1ada078fa1ea8fdfe7e8824e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:25:52 2020 +0200

    config: Drop SWHConfig mixin in favor of the load_from_envvar function
    
    That new function declares exactly what happens today during our configuration
    loading use. Loads and parses the yaml configuration file out of the
    SWH_CONFIG_FILENAME environment variable. Allowing eventual dict enrichment
    with default configuration if provided.
    
    Related to T1532

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

tenma accepted this revision.Tue, Sep 22, 11:45 AM
ardumont updated this revision to Diff 14127.Tue, Sep 22, 1:15 PM

Deprecate SWHConfig mixin in favor of the load_from_envvar function

This will allow to be incremental in the change without failing any builds.
This removal will happen once every impacted clients of this change are
handled.

ardumont retitled this revision from config: Drop SWHConfig mixin in favor of the load_from_envvar function to config: Deprecate SWHConfig mixin in favor of the load_from_envvar function.Tue, Sep 22, 1:16 PM

Build is green

Patch application report for D3965 (id=14127)

Rebasing onto 56d505f34a...

Current branch diff-target is up to date.
Changes applied before test
commit 99cd309c1e616849a3010db80cd017294c94c062
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:25:52 2020 +0200

    config: Deprecated SWHConfig in favor of load_from_envvar function
    
    That new function declares exactly what happens today during our configuration
    loading (for loader/lister/indexer/... and rpc services in general not for the
    CLIs which will be dealt with later).
    
    Loads and parses the yaml configuration file out of the SWH_CONFIG_FILENAME
    environment variable. Allowing eventual dict enrichment with default
    configuration if provided.
    
    Related to T1532

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

ardumont updated this revision to Diff 14129.Tue, Sep 22, 1:33 PM

Fix one-char indent off (by mistake)

Build is green

Patch application report for D3965 (id=14129)

Rebasing onto 56d505f34a...

Current branch diff-target is up to date.
Changes applied before test
commit 0e03218c4c3b5562223b3f310825b5a367ef8a0d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 16 15:25:52 2020 +0200

    config: Deprecated SWHConfig in favor of load_from_envvar function
    
    That new function declares exactly what happens today during our configuration
    loading (for loader/lister/indexer/... and rpc services in general not for the
    CLIs which will be dealt with later).
    
    Loads and parses the yaml configuration file out of the SWH_CONFIG_FILENAME
    environment variable. Allowing eventual dict enrichment with default
    configuration if provided.
    
    Related to T1532

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

ardumont added inline comments.Tue, Sep 22, 2:45 PM
swh/core/config.py
327

to be complete, here is @tenma's suggestion:

@attr.s
class Config:
     conf = attr.ib(factory=dict)
ardumont planned changes to this revision.Tue, Sep 22, 2:56 PM