Add sensors to Google Drive#156167
Conversation
home-assistant
Bot
added
cla-signed
has-tests
integration: google_drive
new-platform
labels
|
Hey there @tronikos, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds sensor platform support to the Google Drive integration, enabling users to monitor their Google Drive storage usage. The changes introduce four new diagnostic sensors (total available storage, used storage, used storage in Drive, and used storage in Drive Trash) that update every 6 hours.
Key changes:
- Added sensor platform with four storage quota sensors
- Created coordinator for data updates with 6-hour polling interval
- Implemented base entity class for Google Drive entities
- Modified backup agent to refresh coordinator after uploads
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| homeassistant/components/google_drive/sensor.py | New sensor platform with four storage quota sensors |
| homeassistant/components/google_drive/coordinator.py | New data update coordinator for fetching storage quota data |
| homeassistant/components/google_drive/entity.py | Base entity class for Google Drive with device info |
| homeassistant/components/google_drive/api.py | Added StorageQuotaData dataclass and async_get_storage_quota method |
| homeassistant/components/google_drive/init.py | Integrated coordinator and sensor platform setup |
| homeassistant/components/google_drive/backup.py | Updated to use coordinator and trigger refresh after uploads |
| homeassistant/components/google_drive/const.py | Added SCAN_INTERVAL constant (6 hours) |
| homeassistant/components/google_drive/strings.json | Added entity translations for sensors |
| tests/components/google_drive/test_sensors.py | Tests for sensor entities using snapshots |
| tests/components/google_drive/conftest.py | Added mock for get_user API call |
| tests/components/google_drive/snapshots/ | Generated test snapshots |
There was a problem hiding this comment.
You need to update quality_scale.yaml
There was a problem hiding this comment.
I updated it, but now we have a silver badge. I'll add diagnostics as a follow-up to get back to platinum.
| super().__init__(coordinator=coordinator) | ||
| self.entity_description = description | ||
| self._attr_unique_id = ( | ||
| f"{slugify(coordinator.config_entry.unique_id)}_{description.key}" |
There was a problem hiding this comment.
why do we slugify? The danger here is that slugify also is an external library, so once that changes its behavior (what can happen), the unique id would change
There was a problem hiding this comment.
For other place in this integration, we use slugify to provide a unique id. For consistency, I used the same implementation.
There was a problem hiding this comment.
The more I think about this line, the less comfortable I feel with how the unique_id is currently handled in this integration. At the moment it’s set unique ID for config entry to the user’s email address (
backup integration include agent ID in diagnostics, so it also export e-mail address), logs, or even entity registry dumps. That’s not ideal from a privacy or data-leak perspective.
It would probably be worth migrate the config entry to use a different identifier e.g. hash of e-mail address or sub field. Google recommends use of sub field for unique ID.
https://developers.google.com/identity/openid-connect/openid-connect
I would be happy for any advice on which approach to choose.
There was a problem hiding this comment.
For now, i updated code to use local variant of slugify, so it is a bit safer, as we don't depends on third party library. In our case, we don't need to handle accented or non-Latin characters or other edge cases, as e-mail address is already normalized.
There was a problem hiding this comment.
Use of sub field may be problematic as it requires new scope 'openid', but in this case, we can use permissionId too.
https://developers.google.com/workspace/drive/api/reference/rest/v3/User
| @pytest.mark.usefixtures( | ||
| "entity_registry_enabled_by_default", | ||
| "setup_integration", | ||
| ) | ||
| async def test_sensor_unknown_when_unlimited_plan( | ||
| hass: HomeAssistant, | ||
| entity_registry: er.EntityRegistry, | ||
| mock_api: MagicMock, | ||
| freezer: FrozenDateTimeFactory, | ||
| ) -> None: | ||
| """Test the total storage are unknown when the user is on an unlimited plan.""" | ||
| mock_api.get_user = AsyncMock( | ||
| return_value={ | ||
| "storageQuota": { | ||
| "limit": None, | ||
| "usage": "100", | ||
| "usageInDrive": "50", | ||
| "usageInTrash": "10", | ||
| } | ||
| } | ||
| ) | ||
| freezer.tick(SCAN_INTERVAL) | ||
| async_fire_time_changed(hass) | ||
| await hass.async_block_till_done() | ||
| assert ( | ||
| entity_entry := entity_registry.async_get( | ||
| "sensor.testuser_domain_com_total_available_storage" | ||
| ) | ||
| ) | ||
| assert (state := hass.states.get(entity_entry.entity_id)) | ||
| assert state.state == "unknown" |
There was a problem hiding this comment.
I am not sure I like this behavior, would it make sense to just not create the entity?
There was a problem hiding this comment.
I was thinking about that too, but since this is a diagnostic entity, I felt it’s better to always have it available - it can help with troubleshooting when something goes wrong.
I’ve updated the code so that the entity won’t be created if the user doesn’t have any limits.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| super().__init__(coordinator=coordinator) | ||
| self.entity_description = description | ||
| self._attr_unique_id = ( | ||
| f"{_slugify(coordinator.config_entry.unique_id)}_{description.key}" |
There was a problem hiding this comment.
I think I'd be okay with just the unique id in here, or I am missing something
There was a problem hiding this comment.
We had a discussion about that earlier: #156167 (comment)
|
@Ronikos You're the final piece of the puzzle. I'm waiting for your reviews. Then I'll take on implementing |
|
|
||
| DOMAIN = "google_drive" | ||
|
|
||
| SCAN_INTERVAL = timedelta(minutes=5) |
There was a problem hiding this comment.
This seems too frequent. The sensors are only expected to be updated after a backup (typically daily or weekly) or after a manual delete (uncommon). How about going with every few hours? In an earlier commit you had every 6h which seems more reasonable. Whoever wants more frequent can call the homeassistant.update_entity action.
There was a problem hiding this comment.
This Google Drive account may not be only used to store HA backups. Other interactions are likely to happen more frequently and should be taken into account.
As long as we're not running into rate-limiting issues I'd say a 5 minute interval is fine. (Or at the very least keep the default more often than hourly).
There was a problem hiding this comment.
I find a 5 minute interval too excessive. Sure other uploads outside HA could increase your quota usage but do regular users care to track it within minutes? The example in the docs notifies you when you are low on storage. Every few hours is more than enough. Whoever wants more frequently can call homeassistant.update_entity in an automation.
There was a problem hiding this comment.
You're merely sharing opinion, which is fine, but other users may not share the same view. If rate limits are not an issue on the API call having an extremely low polling rate should not be chosen.
Requiring users to take convoluted steps of implementing an automation to trigger polling at a different rate for any integration where no sensible default was chosen is just the wrong way of going about this.
There was a problem hiding this comment.
We generally avoid aggressive polling intervals (like 5 minutes) for data that is not critical for immediate, real-time automation (such as a motion sensor or light switch). Storage quota is a "slow-moving" metric.
The homeassistant.update_entity action is the documented, standard mechanism for the minority of users who require non-standard polling frequencies; it is not a workaround. See e.g. https://www.home-assistant.io/integrations/speedtestdotnet/
@mik-laj Please update the interval to proceed. The shortest I'd accept is 1h.
There was a problem hiding this comment.
Six hours sounds good to me. Most users perform one backup per day, so if someone monitors free space to perform backups, everything will work because we'll have notifications four times more often than backups are performed.
This integration does not have services that allow us to create files on Google, so we do not have to worry about other changes on Google Drive.
If someone wants to monitor disk space, they can manually refresh the sensor, but I also don't think that's a common use case to justify changing the interval for all users. The main purpose of this integration is to provide backups, and for that use case, 6 hours is adequate.
| status: exempt | ||
| comment: No data to diagnose. | ||
| devices: done | ||
| diagnostics: todo |
There was a problem hiding this comment.
To avoid downgrading the quality scale, can you add diagnostics? We could make an exception adding 2 platforms. Or you could add an empty diagnostics in a separate PR that we can merge first.
There was a problem hiding this comment.
I added diagnostics in this PR.
| "name": "Used storage" | ||
| }, | ||
| "storage_used_in_drive": { | ||
| "name": "Used storage in Drive" |
There was a problem hiding this comment.
Is this for files uploaded by HA?
There was a problem hiding this comment.
This applies to the entire Google Drive, but it's a good idea. I've added a new sensor that counts the size of backups for our HA instance.
mik-laj
force-pushed
the
google-drive-sensors
branch
from
f7a13ba to
2b3887c
Compare
Breaking change
Proposed change
A few diagnostic sensors that helps Google Drive related problems. This way, we also have a link to the backup folder. Previously, the link was displayed only once after the backup setup was complete.
https://github.com/orgs/home-assistant/discussions/1608
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: