Migrate file integration to config entry #116861
Conversation
home-assistant
bot
added
breaking-change
cla-signed
config-flow
|
Hey there @fabaff, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
jbouwh
changed the title
|
Too much for a single PR I think. |
Yeah, I know. I'll see if I can pop some parts out. |
|
At least the change to config flow and the new NotifyEntity should be in different PRs but I didn't look too carefully so maybe there is something else too which can go into a preliminary PR |
MartinHjelmare
changed the title
jbouwh
changed the title
|
@gjohansson-ST I Have removed the implemention of the new entity platform. Disadvantage is that we cannot fase out the old service with the yaml clean up if the follow up PR is merged later. So now I need to use discovery to set up the legacy service from the entry data. Will split up in 2 parts: |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Thnx! |
| Platform.NOTIFY, | ||
| DOMAIN, | ||
| config, | ||
| {}, |
There was a problem hiding this comment.
The last parameter should be the original config passed to async_setup.
| for domain, items in config.items(): | ||
| for item in items: | ||
| if item[CONF_PLATFORM] == DOMAIN: | ||
| item[CONF_PLATFORM] = domain |
There was a problem hiding this comment.
I'd not modify the original config but make a copy.
| else: | ||
| text = f"{message}\n" | ||
| file.write(text) | ||
| except Exception as exc: |
| @@ -75,6 +86,7 @@ def __init__( | |||
| self._file_path = file_path | |||
| self._attr_native_unit_of_measurement = unit_of_measurement | |||
| self._val_tpl = value_template | |||
| self._attr_unique_id = slugify(f"{name}_{file_path}") | |||
There was a problem hiding this comment.
How do we know that name is unique? Maybe use the config entry id instead?
There was a problem hiding this comment.
We have a check when we import or add on name and file_path
There was a problem hiding this comment.
I don't think that is a good approach. The user should be able to name to whatever they want. That shouldn't be part of our integration logic.
|
|
||
| FILE_SENSOR_SCHEMA = vol.Schema( | ||
| { | ||
| vol.Optional(CONF_NAME, default=DEFAULT_NAME): TEXT_SELECTOR, |
There was a problem hiding this comment.
Why do we ask for a name?
There was a problem hiding this comment.
Because the file integration supports multiple sensors on the same file (mentioned in the docs)
There was a problem hiding this comment.
We could add an incrementing suffix to the filename on duplicate hit, as done for entity_id, and use that as default name. Let the user rename as normal.
|
Follow up on late review: #117210 |
Breaking change
The notify services for the
fileintegration now require that the file path is an allowed path.Users should check the accessed file is in the allowlist_external_dirs to ensure their automation's keep working.
Proposed change
This PR will:
notifyservices cannot access files if they are not added to the allowlist_external_dirs.This PR prepares for another PR to open soon:
notifyentity for everyfilenotify service.Screenshots
Final results after #117210 and #117215:
Type of change
Additional information
Checklist
ruff format 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.To help with the load of incoming pull requests: