Skip to content

refactor: move hathor settings to yaml configuration files [part 1/2]#593

Merged
glevco merged 13 commits intomasterfrom
refactor/settings
May 18, 2023
Merged

refactor: move hathor settings to yaml configuration files [part 1/2]#593
glevco merged 13 commits intomasterfrom
refactor/settings

Conversation

@glevco
Copy link
Contributor

@glevco glevco commented May 10, 2023

Acceptance Criteria

  • Install pyyaml dependency (the go to, canonical yaml implementation for python, repo)
  • Add utility methods to HathorSettings named tuple to create it from a dict or a yaml file, leveraging pydantic for validation
  • Create yaml files to substitute python config files for all network settings (mainnet, testnet, and unitttests).
  • Update get_settings.HathorSettings() to get settings from yaml files instead of python modules.
    • First, it tries to get a python module settings, and logs a deprecation warning if it's found. Then, it gets the yaml
    • This change should be backwards compatible with our current code. A breaking change removing support for settings via python modules will be introduced in a future release.
  • Implement tests to make sure the yaml versions of settings are equal to their python versions, during the migration.

@glevco glevco self-assigned this May 10, 2023
@glevco glevco force-pushed the refactor/settings branch 8 times, most recently from 31c24a8 to 103e8a5 Compare May 11, 2023 07:44
@glevco glevco changed the title refactor: hathor settings refactor: move hathor settings to yaml configuration files May 11, 2023
@glevco glevco force-pushed the refactor/settings branch 4 times, most recently from 9d41d0a to 49cd371 Compare May 11, 2023 16:00
@glevco glevco requested a review from pedroferreira1 May 11, 2023 16:09
@glevco glevco marked this pull request as ready for review May 11, 2023 16:09
@glevco glevco requested review from jansegre and msbrogli as code owners May 11, 2023 16:09
@glevco glevco removed the request for review from jansegre May 11, 2023 16:16
@glevco glevco changed the title refactor: move hathor settings to yaml configuration files refactor: move hathor settings to yaml configuration files (part 1) May 11, 2023
@glevco glevco changed the title refactor: move hathor settings to yaml configuration files (part 1) refactor: move hathor settings to yaml configuration files [part 1/2] May 11, 2023
@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Merging #593 (83e1391) into master (3b372ac) will decrease coverage by 0.03%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
- Coverage   83.68%   83.65%   -0.03%     
==========================================
  Files         231      232       +1     
  Lines       19622    19675      +53     
  Branches     2687     2695       +8     
==========================================
+ Hits        16421    16460      +39     
- Misses       2608     2619      +11     
- Partials      593      596       +3     
Impacted Files Coverage Δ
hathor/conf/get_settings.py 55.00% <56.52%> (-27.61%) ⬇️
hathor/conf/settings.py 97.08% <84.61%> (-2.92%) ⬇️
hathor/builder/cli_builder.py 71.82% <100.00%> (+0.15%) ⬆️
hathor/conf/__init__.py 100.00% <100.00%> (ø)
hathor/utils/pydantic.py 100.00% <100.00%> (ø)
hathor/utils/yaml.py 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@glevco
Copy link
Contributor Author

glevco commented May 12, 2023

Based on @msbrogli's comments, I decided to keep HathorSettings as a NamedTuple and just leverage pydantic's validation to instantiate it from a yaml file, instead of actually changing HathorSettings to pydantic.BaseModel. Done in 10ac376

msbrogli
msbrogli previously approved these changes May 12, 2023
@glevco
Copy link
Contributor Author

glevco commented May 17, 2023

@jansegre

I like the proposed new format. But for the sake of minimizing migration issues, I would rather have this PR still try to load a settings module using the old format and emit a deprecation warning.

Makes sense, done in e0c35ad. I used HATHOR_CONFIG_YAML as you suggested, and also added an equivalent CLI option for it. I have updated the Acceptance Criteria accordingly.

Another point is that most of the time I customize the settings locally, and also when we customize it for our deploys, we inherit a specific config (mainnet/testnet) and alter only a few properties. This is particularly convenient for when there are new parameters, such that the customized settings don't need any change. I think we should consider supporting some settings inherit mechanism before deprecating the old settings (it doesn't have to be on this PR).

I created an issue for that in #614. I left it in the To Do column of our project I'll do it as soon as I close some of the current tasks.

@glevco glevco requested review from jansegre and msbrogli May 17, 2023 19:50
jansegre
jansegre previously approved these changes May 17, 2023
msbrogli
msbrogli previously approved these changes May 17, 2023
@glevco glevco dismissed stale reviews from msbrogli and jansegre via 83e1391 May 18, 2023 04:33
@glevco glevco force-pushed the refactor/settings branch from e0c35ad to 83e1391 Compare May 18, 2023 04:33
@glevco glevco requested review from jansegre and msbrogli May 18, 2023 04:33
@glevco glevco merged commit 77a9a01 into master May 18, 2023
@glevco glevco deleted the refactor/settings branch May 18, 2023 15:00
@jansegre jansegre mentioned this pull request Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants