Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable overriding values in goth config file #489

Merged
merged 3 commits into from
Apr 15, 2021

Conversation

kmazurek
Copy link
Contributor

@kmazurek kmazurek commented Apr 15, 2021

This is necessary as part of golemfactory/yagna#1235 (i.e. moving integration tests from goth to yagna repos).

Adds the possibility to override certain fields in goth-config.yml dynamically, i.e. when the file gets parsed. This is implemented with an additional parameter to goth/configuration.py#load_yaml which is a list of values to override. Here's an example of one such list:

[
    ('docker-compose.build-environment.binary-path', '/tmp/some_path'),
    ('web-root', '/tmp/another_path'),
]

@kmazurek kmazurek requested a review from azawlocki April 15, 2021 14:35
@kmazurek kmazurek self-assigned this Apr 15, 2021
def test_load_yaml_override_new(default_config_file: Path):
"""Test adding a new field to a YAML config file through overrides."""

test_value = "v0.0.1-rc666"
Copy link
Contributor

@azawlocki azawlocki Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤘


def _apply_overrides(dict_: Dict[str, Any], overrides: List[Override]):
for (dict_path, value) in overrides:
path_list: List[str] = dict_path.split(".")
Copy link
Contributor

@azawlocki azawlocki Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could also allow ints in path_list and then override like this:

    overrides = [("nodes.1.name", "dostawca-1")]  # path_list = ["nodes", 1, "name"]
    config = load_yaml(default_config_file, overrides)
    assert config.containers[1].name == "dostawca-1"

But it's probably not worth doing, if we ever need to identify config elements based on list indices then it would be probably an indication that the relevant part of the configuration tree should be made a dir instead of a list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to limit ourselves to dicts for now. The only use-cases I see for list access in our current config file is for debugging purposes. As such, this can be achieved by editing the file itself, rather than using overrides.
With that said I'm going to mention the limitation to dict in the docstring for load_yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 8f83b54

Copy link
Contributor

@azawlocki azawlocki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice use of dpath!

@kmazurek kmazurek merged commit b320650 into master Apr 15, 2021
@kmazurek kmazurek deleted the km/goth-config-overrides branch April 15, 2021 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants