Split up yaml loaders into multiple files #23774
Conversation
Move parts of yaml loader out of the single large file and start to create the structure of the yaml loaders in Ansible [0]. [0]: https://github.com/ansible/ansible/tree/devel/lib/ansible/parsing/yaml
* Move code around to finish the migration
* Update the mocks so that `open` is patched in
`homeassistant.util.yaml.loader` instead of
`homeassistant.util.yaml`.
* Updated mypy ignores
* Updated external API of `homeasistant.util.yaml`, see below:
Checked what part of the api of `homeassistant.util.yaml` was actually
called from outside the tests and added an `__ALL__` that contains only
these elements.
Updated the tests so that references to internal parts of the API (e.g.
the yaml module imported into `homeassistant.util.yaml.loader`) are
referenced directly from `homeassistant.util.yaml.loader`.
In `tests/test_yaml.py` the import `yaml` refers to
`homeassistant.util.yaml` and `yaml_loader` refers to `~.loader`.
Future work that remains for the next iteration is to create a custom
SafeConstructor and refers to that instead of monkey patching `yaml` with
custom loaders.
ties
force-pushed
the
feature/refactor_yaml
branch
from
684cd42
to
a593603
Compare
|
I think this kind of a tricky PR but think the code gets a lot more structured because of it. It is a purely structural change. I realized that I did not check other test cases (i.e. |
|
a heads up: Part of the complete test suite is currently unstable on my laptop and a Debian Buster machine so I am relying on circleci to give the heads up that all is ok. |
I'll follow that one, thanks for the heads up! I think that I caught all the places directly using
I was aware of that and think this work is orthogonal to that. If this restructuring (and the structure as in Ansible) is actually clearer to other developers. Is there a way to add telemetry so we can see how often these special loaders are actually used? In my opinion some of them seem to have niche use cases. |
I don't think we should expand environment variables in all strings in the config - we shouldn't change the value of any YAML object at all without the user intentionally requesting to do so with a YAML tag. Reason is: There are probably many real use-cases (like shell script component) where they should not be expanded. If we would expand them, that might be a very un-intuitive annoyance. Plus it would be a bad breaking change. Plus I don't see why Another option would be a system esphome uses: substitutions (https://esphome.io/guides/configuration-types.html#config-substitutions) - only keys that are explicitly defined by the user are expanded. Everything else that looks like a substitution but is not defined as one, is not expanded.
Problem is HA templates also use jinja2 - if we evaluate jinja2 at load time it would fundamentally mess with HA templating. If you want to pursue it further, I would suggest opening an architecture issue (https://github.com/home-assistant/architecture). |
|
I was away for the weekend so this comes a bit late - but I'll expand on the environment variables.
Thanks for the quick feedback and the explanation of your considerations! It is a product change I would definitely not consider before having access to telemetry. Changing semantics without a good reason is annoying. I would only consider this for single line strings and only if it conflicts in a limited number of cases. The use case is the general "configure through environment variables" pattern that is the part of the 12 factor app pattern that I appreciate. My use case is building a connection string to influxdb and mysql from the environment variables that docker compose creates. This needs multiple tokens (
That is the way. There are many paths to take (adding a (version) tag to the yaml, only applying to single line strings, only apply to some type of multiline quoting, only apply within quotes, etc). For now I hardcoded the connection string in my config 🙃 |
Breaking Change:
Some implementation details of
homeassistant.util.yamlare no longer public (their path changed). Some of these may not be technically internal. All exports fromhomeassistant.util.yamlthat were accesses from outside the tests are still available using their original name. See commit messages for exact details.API changes:
homessistant.util.yaml.represent_odictis not exposed as part ofhomeassistant.util.yamlbut can be accessed fromhomeassistant.util.yaml.dumper.represent_odicthomeasistant.util.yaml.{NodeListClass, NodeStrClass}are not exposed as parts of `homeassistant.util.homeassistant.util.yamldoes not longer export imported (and monkeypatched) yaml moduleNote that
NodeStrClassdoes not appear to be actually used because the condition in yaml.py#L82 never matches - this is seen in test coverage.Description:
homeassistant.util.yamlhas been split up in preparation for more cleanup, following the structure from Ansible.I intended to add a feature to the yaml loader to expand environment variables in strings (such as in docker-compose.yml). When considering this I noticed that the
homeassistant.util.yamlwas one large monolithic file that monkey patches system yaml.Steps
The first improvement I would like to propose is contained in this PR and splits up the yaml code, following the structure in Ansible where the custom yaml loaders are split up into multiple files with clear roles.
A second step would be to no longer monkey patch
yaml.yamlbut to create a customSafeLoadersuch as in Ansible. Since this is an important change I would propose to do this in a separate iteration.A (optional) third step would be to add "expand environment variables in strings" to make values (e.g. database connection strings) configurable using environment variables that are expanded using the common
${VAR}syntax instead of!!envand templating the value outside of the configuration. Or to use jinja2 for strings in the configuration such as happens in ansible.Checklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
python3 -m script.hassfest.requirements_all.txtby runningpython3 -m script.gen_requirements_all..coveragerc.If the code does not interact with devices: