Add snapshot service to image entity #110057
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
NickM-27
force-pushed
the
image-service
branch
from
68ebd70 to
4a9164c
Compare
8 tasks
NickM-27
force-pushed
the
image-service
branch
2 times, most recently
from
ae0cdba to
1cf1e69
Compare
NickM-27
force-pushed
the
image-service
branch
from
7845a05 to
e1c12c9
Compare
|
@NickM-27 There are merge conflicts, can you please merge or rebase the PR? |
NickM-27
force-pushed
the
image-service
branch
from
4174078 to
558cb35
Compare
|
@emontnemery thanks, rebased 👍 |
frenck
added
the
smash
|
looks like the failing tests are unrelated to these changes |
allenporter
requested changes
Apr 21, 2024
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
allenporter
requested changes
Apr 21, 2024
NickM-27
force-pushed
the
image-service
branch
from
8ccd24e to
1cb31e4
Compare
emontnemery
reviewed
Oct 17, 2024
Co-authored-by: Erik Montnemery <[email protected]>
emontnemery
reviewed
Oct 21, 2024
emontnemery
reviewed
Oct 21, 2024
emontnemery
reviewed
Oct 21, 2024
| # check if we allow to access to that file | ||
| if not hass.config.is_allowed_path(snapshot_file): | ||
| raise HomeAssistantError( | ||
| f"Cannot write `{snapshot_file}`, no access to path; `allowlist_external_dirs` may need to be adjusted in `configuration.yaml`" |
There was a problem hiding this comment.
Please break long strings around max 88 characters per line.
| f"Cannot write `{snapshot_file}`, no access to path; `allowlist_external_dirs` may need to be adjusted in `configuration.yaml`" | ||
| ) | ||
|
|
||
| async with asyncio.timeout(IMAGE_TIMEOUT): |
There was a problem hiding this comment.
What catches TimeoutError? Only HomeAssistantError is allowed to leak out from the service handler.
|
|
||
| # check if we allow to access to that file | ||
| if not hass.config.is_allowed_path(snapshot_file): | ||
| raise HomeAssistantError( |
There was a problem hiding this comment.
Shouldn't this be ServiceValidationError?
https://developers.home-assistant.io/docs/core/platform/raising_exceptions/
| "fields": { | ||
| "filename": { | ||
| "name": "Filename", | ||
| "description": "Template of a filename. Variable available is `entity_id`." |
There was a problem hiding this comment.
Didn't we remove the entity_id variable?
zxdavb
pushed a commit
to zxdavb/hass
that referenced
this pull request
Oct 25, 2024
* Add service definition for saving snapshot of image entity * Add service to image * Add tests for image entity service * Fix tests * Formatting * Add service icon * Formatting * Formatting * Raise home assistant error instead of single log error * Correctly pass entity id * Raise exception from existing exception * Expect home assistant error * Fix services example * Add test for templated snapshot * Correct icon service config * Set correct type for service template * Remove unneeded Co-authored-by: Erik Montnemery <[email protected]> * remove template * fix imports * Update homeassistant/components/image/__init__.py * Apply suggestions from code review --------- Co-authored-by: Erik Montnemery <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed change
Integrations are moving from using the camera.entity to image.entity but there is not feature parity when it comes to the services offered for the image.entity. This PR adds the snapshot service to the image entity so users are able to save the images offline for their own usage.
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: