-
-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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
Add "import" and "include" template features #42244
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @derekatkins.
I've left a comment that needs to be addressed, furthermore, this hits core functionality and tests should be added.
homeassistant/helpers/template.py
Outdated
@@ -1262,7 +1262,7 @@ class TemplateEnvironment(ImmutableSandboxedEnvironment): | |||
|
|||
def __init__(self, hass): | |||
"""Initialise template environment.""" | |||
super().__init__() | |||
super().__init__(loader=jinja2.FileSystemLoader("/config/templates")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not have a statically configured path, but use the actual configuration path of the system instead. Besides, should this be fixed to a templates
subfolder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @frenck . Very good point. How does one determine the actual configuration path?
As for the fixed templates
subfolder, I did that for security reasons to separate out from other config files and ensure one could not, for example, {% include 'secrets.yaml' %}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its in the hass object: hass.config.config_dir
.
I kinda get the security concern, however, if one has access to this level of things, that might be a false sense of security already. The downside of limiting/locking it to the templates
folder, is limiting the user to structure the configuration to their own likings (e.g., packages use).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks. Didn't see config_dir
in the docs!
Well, there is nothing stopping the user from creating a /config/templates/{X,Y,Z}/...
structure for packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, @frenck, that doesn't work:
homeassistant/helpers/template.py:1265: in __init__
super().__init__(loader=jinja2.FileSystemLoader(hass.config.config_dir+"templates"))
E AttributeError: 'NoneType' object has no attribute 'config'
Apparently __init__
is being called with hass=None at some point. I don't know if there is a way to lazy-eval that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I know how to solve this... But it means that the feature will only work if there is a hass env.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frenck -- I have this now working locally based on config_path
, but the code is a little ugly because I can only instantiate the loader if hass is not None, so I have to call super.__init__()
differently. I can push now if you want to see it.
Now to add tests as per your other request; how can I add a "file" in the config path in the test infrastructure? I can do the "stupid" thing and literally open/write/close/unlink -- but is there a better way to push something into the filesystem under the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now believe I have made the config and test changes you requested.
Before you invest more time in this feature, I think that we first discuss if this is a feature that we want, and if it is worth the structural changes to Home Assistant. What would be a use case that is covered? |
@frenck Hi. So I pushed the code that fixes the hard-coded config-dir. I still think the they should live under their own subdirectory tree of the config tree. As for adding tests, how can I mock up the filesystem so that @balloob Subroutines in templates. I'm building a set of automations to handle all my thermostats (I have 12) and want to do the following:
Right now there is no way to write a script that actually returns a value, so I cannot create that "time_bucket_for_thermostat" function. The best I can do is create a hidden Moreover, people have been asking for macros for YEARS. So a better question is: what reasons would there before NOT to support macros? I am curious, short of the test infrastructure, what structural changes are required? I'm running with this change and it works quite well for me in my tests (although I am not, yet, using it in production because I don't want to have to patch after every update). Thanks! |
I must admit, that for all above-listed reasons, I'm like: Why not use a script? As for the "Why not", that is simply answered. Every feature adds maintenance and new possible cases of things that can go wrong. We are not aiming to be able to do everything, we are aiming to have a flexible and uniform set of things that are commonly shared/used. Hence, when adding a new feature the question is asked like balloob did and not the other way around. |
Because a script cannot return a result. You are right that I could probably write the master automation as a python_script, but there are other places where I'd like to use macros in my automations too. For now I've just copy-and-pasted my templates, but honestly it would be nice to be able to define them once.
That's a fair point. As I mentioned above, this feature has been requested for years, going back to at least 2015 when there was a comment that suggested it could be done because jinja supports it. So there are clearly users who want it. I'm happy to jump through the hoops you all want to properly test that we pass the correct loader to jinja, but I also assume that jinja tests themselves that their loaders work. |
As you may be able to tell, I'm a python n00b. With some helpful prodding from @raman325 on Discord he pointed me in the right direction to patch the loader in order to test that the templates render as expected when importing or including expected data. So I have added that test (positive and negative), so show that we get the expected template results when we load what we expect, and we get a TemplateError if we try to load something that doesn't exist. As to why this is useful -- it allows anyone to build macros and de-duplicate any automation or script code by refactoring it and putting it into a single place. I.e., it can simplify a bunch of complicated and repetative templating. |
super().__init__() | ||
if hass: | ||
super().__init__( | ||
loader=jinja2.FileSystemLoader(hass.config.config_dir + "templates") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you tag me in the conversation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, was trying to say that @ raman on HA discord helped me and wanted to thank them. But of course that could be someone completely different. My deepest, sincerest apologies for tagging you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to tag @raman325
Still not clear what can't be done inside a script using variables and choose. Or using the python script integration. Our templates are not supposed to be a programming language. |
Hi. There are several reasons:
{% set ... %} # A bunch of variables get set here in a complicated series of templates
{% if x %}
- service: script.turn_on
entity_id: script.script1
.... (data based on one part of X)
{% else %}
- service: script.turn_on
entity_id: script.script2
.... (different data based on a different part of X)
{% endif %} The only way I have figured out how to do this is by separate automations that call separate scripts based on the underlying type of entity, however....
|
See |
Thanks. That potentially solves ONE of my problems and definitely will let me clean up some of my scripts and automations. Can Still, it doesn't solve the problem of having to repeat templates and variables in different automations due to being necessarily different automations but requiring similar computed data. For example, I have one automation to track thermostats. When any thermostat setting changes (i.e., you change the setting at the thermostat) it triggers the automation, which then has to perform a bunch of template operations to compute the correct backing-store Then I have a different set of automations that kick off due to the time changes to set the thermostat(s) from the |
Considering your questions, I'm pretty sure you are approaching things wrongly, mainly due to the missing functionality of the platform. Missing that knowledge, combined with a vision of your own solution, results in this PR.
This can all be done in scripts.
You don't have to and isn't related to anything in this PR or issue you are describing. Even this PR adds macros, it doesn't change anything about that. I'm even thinking if we (alternatively) should allow/create a possibility to allow one to plug in custom filters in the future (maybe even defined in YAML itself?). Even that is ambiguous, as automation/script variables can provide that functionality (kinda). I'm leaning voting against this change at this point. |
That is certainly a possibility; I've been a software developer for 30 years so sometimes old habits are hard to break, namely if you have to write the same code twice you're doing something wrong.
I'm still at a loss for how to do that without a macro to help me. Let me take my backing-store save and load example. I have an automation that triggers when Next, I need to do the inverse. Originally I was thinking that I would just have a script that would take a look at
But it is -- You can't share partial templates across different scripts or automations. If I want to set variables X, Y, and Z in the same way in multiple templates, I have to copy the same template code into each one. Unless there is yet another platform feature that I am missing that let's me build script A that sets variables so that scripts B, C, and D can see and use those variables?
This is (almost) exactly what I'm trying to do with macros! It would allow me to write a macro that computes a value and then let me use that value computation in multiple scripts. It's not quite as useful to me as I would like because I can't set multiple things at once (e.g, I'd like to have one macro that let's me compute the |
Let's not add this feature. It is going to make things a lot more complicated and things are already complicated enough. Alternatives:
|
With
I don't see a script doing this, and I'm not a python programmer. HA already has a strong commitment to |
Proposed change
Type of change
Example entry for
configuration.yaml
:Additional information
This PR enables the
include
andimport
jinga functionality. It will look in/config/templates
for the imported (or included) templates.import
orinclude
results in stack trace and error #42224Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: