File Path fixes for RPi Camera #12338
Conversation
|
Hi @FrederikBolding, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
homeassistant
added
cla-needed
small-pr
FrederikBolding
changed the title
| @@ -26,6 +26,7 @@ | |||
| CONF_TIMELAPSE = 'timelapse' | |||
| CONF_VERTICAL_FLIP = 'vertical_flip' | |||
|
|
|||
| DEFAULT_FILE_PATH = '' | |||
There was a problem hiding this comment.
I added it prematurely. It was supposed to point somewhere else to prevent the files from being saved in the camera components folder.
| @@ -36,7 +37,7 @@ | |||
| DEFAULT_VERTICAL_FLIP = 0 | |||
|
|
|||
| PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({ | |||
| vol.Optional(CONF_FILE_PATH): cv.string, | |||
| vol.Optional(CONF_FILE_PATH, default=DEFAULT_FILE_PATH): cv.string, | |||
There was a problem hiding this comment.
I guess that with cv.isfile instead of cv.string we could get rid of the checks in setup_platform().
There was a problem hiding this comment.
Yep, seems like it! Will change in a few 😄
FrederikBolding
changed the title
FrederikBolding
changed the title
|
There we go! Let me know if there are any issues! 😄 |
| @@ -78,24 +77,17 @@ def setup_platform(hass, config, add_devices, discovery_info=None): | |||
| CONF_HORIZONTAL_FLIP: config.get(CONF_HORIZONTAL_FLIP), | |||
| CONF_VERTICAL_FLIP: config.get(CONF_VERTICAL_FLIP), | |||
| CONF_FILE_PATH: config.get(CONF_FILE_PATH, | |||
| os.path.join(os.path.dirname(__file__), | |||
| 'image.jpg')) | |||
| hass.config.path('image.jpg')) | |||
There was a problem hiding this comment.
Can you rename it to camera.rpi_camera.jpg ? Makes it clear where the image is from.
There was a problem hiding this comment.
Actually, why do we even do this, wouldn't it make more sense to just use a temporary file when none is given? No reason to put it in the config dir.
There was a problem hiding this comment.
For creating temp files and dirs https://docs.python.org/3/library/tempfile.html
There was a problem hiding this comment.
CONF_FILE_PATH: config.get(CONF_FILE_PATH,
tempfile.NamedTemporaryFile(suffix='.jpg', delete=False).name)
Are you talking about something like this?
I'm a bit unfamiliar with the Home Assistant code-base still, but wouldn't we run into issues with multiple images being saved as Home Assistant is restarted? Because we aren't deleting the used image.
Or is the default generated temp file path saved somehow?
Otherwise, we would have to handle deletion of the temp file when Home Assistant is restarted.
There was a problem hiding this comment.
I wouldn't instantiate it as default param, because that way you would still create a temp file even when CONF_FILE_PATH is specified.
You can listen to EVENT_HOME_ASSISTANT_STOP to run code when Home Assistant stops.
| hass.bus.listen_once(EVENT_HOMEASSISTANT_STOP, delete_temp_file) | ||
|
|
||
| # Check whether the file path has been whitelisted | ||
| if not hass.config.is_allowed_path(file_path): |
There was a problem hiding this comment.
No need to check file path to be allowed if we use a temp file.
|
🎉 🎉 |
ghost
added
Description:
Related issue (if applicable): fixes #10360
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4650
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests pass