Implement config versioning #1058
Conversation
|
@davide-f have drafted a possible implementation. Could you please have a look? Have added one more field to the version number to track the changes in the config. Not sure it's the most elegant solution, though. A possible improvement could be to add a check on the differences between the local config and |
config.default.yaml
Outdated
| @@ -2,7 +2,7 @@ | |||
| # | |||
| # SPDX-License-Identifier: CC0-1.0 | |||
|
|
|||
| version: 0.3.0 | |||
| version: 0.3.0.0 | |||
config.tutorial.yaml
Outdated
| @@ -2,7 +2,7 @@ | |||
| # | |||
| # SPDX-License-Identifier: CC0-1.0 | |||
|
|
|||
| version: 0.3.0 | |||
| version: 0.3.0.0 | |||
ekatef
force-pushed
the
implement_config_versioning
branch
from
39d050c
to
41b4b62
Compare
Done 🙂 Thanks a lot for the review! I'm thinking what would be the best way to remind that a release is needed every time, when the config structure gets changed. Adding a point for that to PR template sounds like not a really good idea, but I don't have better ones at the moment. What is your perspective on that? |
There was a problem hiding this comment.
Great @ekatef :D
I've added a minor comment. Moreover there is a minor merge conflict to fix.
This PR is ready to go afterwards. If you like to include these minor changes, that would be nice but the functionality is there.
Afterwards, if CI passes, feel free to merge yourself :)
Snakefile
Outdated
| @@ -3,6 +3,7 @@ | |||
| # SPDX-License-Identifier: AGPL-3.0-or-later | |||
|
|
|||
| import sys | |||
| import yaml | |||
There was a problem hiding this comment.
Can we drop this line? it seems is not needed
There was a problem hiding this comment.
True, have forgotten to remove it. Thanks!
scripts/_helpers.py
Outdated
| def check_config_version(config): | ||
| """ | ||
| Check that a version of the local config.yaml matches to the actual config | ||
| version as defined in config.default.yaml. | ||
| """ | ||
|
|
||
| # using snakemake capabilities to deal with yanl configs | ||
| with open("config.default.yaml", "r") as f: | ||
| actual_config = yaml.safe_load(f) | ||
| actual_config_version = actual_config.get("version") |
There was a problem hiding this comment.
I'd propose to have the file path as parameter here, like:
| def check_config_version(config): | |
| """ | |
| Check that a version of the local config.yaml matches to the actual config | |
| version as defined in config.default.yaml. | |
| """ | |
| # using snakemake capabilities to deal with yanl configs | |
| with open("config.default.yaml", "r") as f: | |
| actual_config = yaml.safe_load(f) | |
| actual_config_version = actual_config.get("version") | |
| def check_config_version(config, fp_config="config.default.yaml"): | |
| """ | |
| Check that a version of the local config.yaml matches to the actual config | |
| version as defined in config.default.yaml. | |
| """ | |
| # using snakemake capabilities to deal with yanl configs | |
| with open(fp_config, "r") as f: | |
| actual_config = yaml.safe_load(f) | |
| actual_config_version = actual_config.get("version") |
Co-authored-by: Davide Fioriti <[email protected]>
@davide-f fixed! Thank you 😄 Ok, then I'm merging it. As a comment, the current implementation writes an error in logs in case the config is outdated, but don't terminate execution. Leaving that for now, while it can be improved further when introducing better management of the configs. |
Changes proposed in this Pull Request
Implements a check to ensure that the current local
config.yamlis consistent with the version defined inconfig.default.yaml.Checklist
envs/environment.yamlanddoc/requirements.txt.config.default.yamlandconfig.tutorial.yaml.test/(note tests are changing the config.tutorial.yaml)doc/configtables/*.csvand line references are adjusted indoc/configuration.rstanddoc/tutorial.rst.doc/release_notes.rstis amended in the format of previous release notes, including reference to the requested PR.