-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
File add read_file action with Response #139216
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
base: dev
Are you sure you want to change the base?
Conversation
Hey there @fabaff, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
"services": { | ||
"read_file": { | ||
"name": "Read File", | ||
"description": "Reads a file and return the contents.", |
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.
-> "… and returns …" (third-person for both verbs :-))
In all three name
keys use sentence-casing:
- "Read file"
- "File name"
- "File encoding"
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.
thank you, i've updated.
yaml_content = yaml.safe_load(file_content) | ||
if isinstance(yaml_content, dict): | ||
return yaml_content | ||
return {"yaml": yaml_content} |
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 are we putting it in a dictionary?
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.
When the YAML file contains a list of elements - for example in the fixture file file_read_list.yaml - a list is returned. A list is not serializable as a dict (at least as the top most element) and a runtime error gets generated when HA tries to serialize. So placing it in a dict like this allows is to be serialized. I found a similar result if the YAML file contained only a string (returns a str). So if safe_load does not return a dict we need to create a dict.
I will add a comment to the code. Perhaps we should call the dict element something other than "yaml" like "data"?
We could also always create a dict regardless of the content type, this may be more consistent and simpler. I did not do that because most YAML file are typically dicts and it would add another level to navigate when processing the response.
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 like data
as a key more than yaml
. But my main concern is different: I don't like that users would have to guess how the response is formatted. I think that if it is a dict, you should also put in into data
, so that it is consistent.
That also prevents degenerate cases, like how do you differentiate a list from a dict with a single key data
and a list as a value.
Edit: I didn't see your edit when writing my response, but yes, that is what I'm proposing. I think consistency here is more important.
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.
Ok, makes sense to me. We should also do the same thing for the json branch? So the parsing result will always be in response["data"]
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.
Yes, let's keep it the same for all.
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.
updated
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 this pull request @PeteRager 👍
I do think this is something I want to check in a core architectural meeting to see if this is something we would like to have on core or not mostly because this can be a potential security risk.
I'll check and get back to you once I have more information. For now, I'm marking this PR as a draft.
../Frenck
Thanks. I am restricting it to paths registered in allowlist_external_dirs |
Yes, I've seen. That is already good. Just want to make sure the rest agrees on it as well. ../Frenck |
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Keep |
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Keep |
Proposed change
Add action to allow reading a file and returning the file content as the response to be used in an automation. This allows loading content or metadata; reading incoming files; and using this data in an automation. Initially, the action supports loading JSON or YAML files.
Example action call:
Sample result
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
.To help with the load of incoming pull requests: