Skip to content

Refactor Synology entries to allow not fetching the API when it's disabled + add security binary sensor#35565

Merged
Quentame merged 25 commits into
home-assistant:devfrom
Quentame:synology_dsm/disable-entries-and-not-fetch
Jun 2, 2020
Merged

Refactor Synology entries to allow not fetching the API when it's disabled + add security binary sensor#35565
Quentame merged 25 commits into
home-assistant:devfrom
Quentame:synology_dsm/disable-entries-and-not-fetch

Conversation

@Quentame
Copy link
Copy Markdown
Member

@Quentame Quentame commented May 12, 2020

Breaking change

  • The following sensors are now binary sensors:
    • disk_exceed_bad_sector_thr
    • disk_below_remain_life_thr
  • The following sensors have been removed:
    • volume type (RAID, SHR ...)
    • disk name (Drive [X])
    • disk device (/dev/sd[Y])
  • The disk and volume sensors have been replaced: sensor.synology_status_sda to sensor.synology_drive_1_status, sensor.synology_average_disk_temp_volume_1 to sensor.synology_volume_1_average_disk_temp, etc.

Proposed change

  • add Synology DSM Security binary sensor (enabled by default)
  • use device name instead of id in names
  • add device type to name
  • show disk manufacturer, model and firmware version in devices
  • some entries are disabled by default (entity_registry_enabled_default)
  • binary sensor + sensor uses device_class when possible
  • do not fetch a concerned API if all entries of it are disabled

Changes:

  • entity unique_id now uses key instead of label
  • entity entity_id changes for disk and volume: example from sensor.synology_status_sda to sensor.synology_drive_1_status, or from sensor.synology_average_disk_temp_volume_1 to sensor.synology_volume_1_average_disk_temp
  • now binary sensor:
    • disk_exceed_bad_sector_thr
    • disk_below_remain_life_thr
  • removed sensor:
    • volume type (RAID, SHR ...)
    • disk name (Drive [X])
    • disk device (/dev/sd[Y])

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@probot-home-assistant
Copy link
Copy Markdown

Hey there @ProtoThis, mind taking a look at this pull request as its been labeled with a integration (synology_dsm) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@Quentame Quentame changed the title Refactor Synology entries to allow not fetching the API when it's disabled + add Security API Refactor Synology entries to allow not fetching the API when it's disabled + add security binary sensor May 12, 2020
@Quentame Quentame mentioned this pull request May 12, 2020
20 tasks
@Quentame
Copy link
Copy Markdown
Member Author

Devices uses device name & type:
Capture d’écran 2020-05-13 à 01 26 53
Before: Synology NAS (sda), Synology NAS (sdb), Synology NAS (sdc), Synology NAS (volume_1), and manufacturer and model were the same as the first device.


Device with one entity disabled by default + 2 first entities as binary (inactif means inactive):
Capture d’écran 2020-05-13 à 01 28 12


Main device with few entities enabled + the new secured binary sensor (sécurisé means secured):
Capture d’écran 2020-05-13 à 01 28 49

Main device with all entities disabled, SynoCoreUtilization + SynoCoreSecurity APIs are not fetched:
Capture d’écran 2020-05-13 à 01 27 38

@Quentame Quentame requested a review from bdraco May 12, 2020 23:54
Comment thread homeassistant/components/synology_dsm/__init__.py Outdated
Comment thread homeassistant/components/synology_dsm/__init__.py Outdated
Comment thread homeassistant/components/synology_dsm/sensor.py
@MartinHjelmare
Copy link
Copy Markdown
Member

Sounds like lots of things are going on in this PR. I would advice to split each thing into its own PR. If we need to write + in the PR header to sum up the changes, it's a sign that we should split the PR.

@Quentame Quentame force-pushed the synology_dsm/disable-entries-and-not-fetch branch from 9c71af9 to 06a0eab Compare May 13, 2020 21:32
@Quentame Quentame force-pushed the synology_dsm/disable-entries-and-not-fetch branch 4 times, most recently from 00317fe to 2071a67 Compare May 15, 2020 00:58
Comment thread homeassistant/components/synology_dsm/__init__.py Outdated
Comment thread homeassistant/components/synology_dsm/__init__.py Outdated
@Quentame Quentame force-pushed the synology_dsm/disable-entries-and-not-fetch branch from 2071a67 to e135424 Compare May 20, 2020 21:45
@Quentame Quentame force-pushed the synology_dsm/disable-entries-and-not-fetch branch 2 times, most recently from 07d651b to 9dc62e4 Compare May 21, 2020 07:04
@Quentame Quentame requested a review from bdraco May 21, 2020 08:48
@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 25, 2020

I haven't forgotten about this. I'll get to the review as soon as 0.110 items calm down

@Quentame Quentame force-pushed the synology_dsm/disable-entries-and-not-fetch branch from fa038b5 to 6e0c771 Compare June 1, 2020 09:31
@Quentame Quentame requested a review from bdraco June 1, 2020 14:01
Comment thread homeassistant/components/synology_dsm/__init__.py Outdated
@Quentame Quentame requested a review from bdraco June 2, 2020 00:16
Comment thread homeassistant/components/synology_dsm/__init__.py Outdated
Comment thread homeassistant/components/synology_dsm/__init__.py Outdated
Comment thread homeassistant/components/synology_dsm/__init__.py
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jun 2, 2020

The subscribe code looks good 🍾

Just a few minor cleanups and I'll start testing.

@Quentame Quentame requested a review from bdraco June 2, 2020 06:43
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jun 2, 2020

Screen Shot 2020-06-02 at 9 30 21 AM

Screen Shot 2020-06-02 at 9 30 15 AM

Screen Shot 2020-06-02 at 9 30 09 AM

Testing looks good on my test instance

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jun 2, 2020

Screen Shot 2020-06-02 at 9 33 30 AM

I had some entities that didn't get migrated with the unique id changes

Screen Shot 2020-06-02 at 9 35 25 AM

@Quentame
Copy link
Copy Markdown
Member Author

Quentame commented Jun 2, 2020

Screen Shot 2020-06-02 at 9 30 09 AM

Maybe I should add a " - " between the disk/volume name and its type 🤔

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jun 2, 2020

Screen Shot 2020-06-02 at 9 36 37 AM

Screen Shot 2020-06-02 at 9 36 33 AM

Screen Shot 2020-06-02 at 9 36 26 AM

Screen Shot 2020-06-02 at 9 36 22 AM

Screenshots of the ones that didn't migrate successfully

@Quentame
Copy link
Copy Markdown
Member Author

Quentame commented Jun 2, 2020

Screen Shot 2020-06-02 at 9 36 37 AM Screen Shot 2020-06-02 at 9 36 33 AM Screen Shot 2020-06-02 at 9 36 26 AM Screen Shot 2020-06-02 at 9 36 22 AM

Screenshots of the ones that didn't migrate successfully

Those are normal, those sensors are either removed or changed from sensor to binary_sensor

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jun 2, 2020

Those are normal, those sensors are either removed or changed from sensor to binary_sensor

I adjusted the breaking change section to reflect that. Please check

@Quentame
Copy link
Copy Markdown
Member Author

Quentame commented Jun 2, 2020

Those are normal, those sensors are either removed or changed from sensor to binary_sensor

I adjusted the breaking change section to reflect that. Please check

Thanks, good 👍

@Quentame
Copy link
Copy Markdown
Member Author

Quentame commented Jun 2, 2020

Just adding the caret before merging

@Quentame
Copy link
Copy Markdown
Member Author

Quentame commented Jun 2, 2020

Python3.7 failing test not related to this PR.

Related to buienradar integration test_camera.test_expire_delta
See: https://dev.azure.com/home-assistant/Core/_build/results?buildId=45847&view=logs&j=67510a0b-4400-5e74-cb95-9572caf5ca54&t=f8fe93fb-5ebb-59cc-455e-c0f2d9ad3a42&l=278

Made an issue of it #36382

@Quentame Quentame merged commit 26cbca1 into home-assistant:dev Jun 2, 2020
@Quentame Quentame deleted the synology_dsm/disable-entries-and-not-fetch branch June 2, 2020 16:22
Quentame added a commit to hacf-fr/home-assistant-core that referenced this pull request Oct 31, 2021
Quentame added a commit that referenced this pull request Oct 31, 2021
natekspencer pushed a commit to natekspencer/home-assistant-core that referenced this pull request Nov 4, 2021
meichthys pushed a commit to meichthys/core that referenced this pull request Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants