Add reboot and shutdown service to synology_dsm by mib1185 · Pull Request #42697 · home-assistant/core

mib1185 · 2020-11-01T01:16:25Z

Proposed change

This PR make use of new features in python-synology lib v1.0.0 and add services to reboot and shutdown a DSM.
Rebase will be done, as soon #42539 is merged.
As side effect #41701 will be fixed.

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

This PR fixes or closes issue: fixes SynologyDSM shows all unavailable after a schedule power down/up #41701
This PR is related to issue:
Link to documentation pull request: Add reboot and shutdown service to synology_dsm home-assistant.io#15511

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:

Documentation added/updated for www.home-assistant.io

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

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

mib1185 · 2020-11-01T01:20:40Z

open todos for me:

update documentation and create PR
write tests for services
rebase after Add verify_ssl option for connection setup to synology_dsm #42539 is merged.

Quentame

Some questions otherwise seems nice.

mib1185 · 2020-11-08T12:58:03Z

@Quentame are there any open concerns from your side about this PR?
I really don't want to annoy you, but would like to complete this PR 😉

Quentame

LGMT

Quentame · 2020-11-11T18:38:29Z

Sorry for being so long, at first I would like to test, even if everything looks pretty nice. Then, I got some days off and I felt the need to give a break at code.
But hey !
I'm back 😉

mib1185 · 2020-11-11T18:46:48Z

ahhh ... it's ok 😉 you don't owe me an explanation 😉
But yeah, it's nice to see you again here 🥳

* 'dev' of https://github.com/home-assistant/core: (552 commits) Use media player image proxy for roku media browser (home-assistant#43070) Add Shelly support for REST sensors (home-assistant#40429) Add system health section for the Supervisor (home-assistant#43074) Simplify distance conversion (home-assistant#43107) Add initial rest query params (home-assistant#42198) Bump pycsspeechtts to 1.0.4 (home-assistant#43105) Bump greeclimate to 0.9.6 (home-assistant#43106) Add reboot and shutdown service to synology_dsm (home-assistant#42697) Bump up ZHA dependencies (home-assistant#43104) Modify wait timeout in stream (home-assistant#42794) Bumped version to 0.117.6 Fix Plex auth issues by setting header (home-assistant#43081) Bump gTTS-token to 1.1.4 (home-assistant#43015) Bump pwmled to v1.6.7 (home-assistant#42903) Remove unneeded state restoration of the physical device in rpi_gpio_pwm integration (home-assistant#42804) Update frontend to 20201111.0 (home-assistant#43101) Show media progress in sisyphus (home-assistant#42996) Use internal url for Squeezebox if possible (home-assistant#43089) Add watchdog to reset nest subscriber (home-assistant#43034) Bump hatasmota to 0.0.29 (home-assistant#43013) ...

MartinHjelmare

Please address the comments in a new PR. Thanks!

MartinHjelmare · 2020-11-12T05:06:46Z

+        if not self.system:
+            _LOGGER.debug("async_reboot - System API not ready: %s", self)
+            return
+        self._hass.async_add_executor_job(self.system.reboot)


Why do we not await this executor job?

MartinHjelmare · 2020-11-12T05:07:36Z

        await api.async_setup()
-    except SynologyDSMRequestException as err:
+    except (SynologyDSMLoginFailedException, SynologyDSMRequestException) as err:
+        _LOGGER.debug("async_setup_entry - Unable to connect to DSM: %s", str(err))


We start logging messages with capital letter.

We don't need to copy exceptions to string when logging them. They have a __str__ magic method already.

MartinHjelmare · 2020-11-12T05:08:58Z

+
+    async def service_handler(call: ServiceCall):
+        """Handle service call."""
+        _LOGGER.debug(


This is already logged by the core.

MartinHjelmare · 2020-11-12T05:10:09Z

+            await dsm_api.system.shutdown()
+
+    for service in SERVICES:
+        _LOGGER.debug(


Logging the services we register seems not needed. There's no variation.

MartinHjelmare · 2020-11-12T05:10:43Z

+            )
+            return
+
+        _LOGGER.info("%s DSM with serial %s", call.service, serial)


Max debug level.

MartinHjelmare · 2020-11-12T05:11:28Z

+            await self._hass.async_add_executor_job(
+                self.dsm.update, self._with_information
+            )
+            async_dispatcher_send(self._hass, self.signal_sensor_update)


Please only wrap the line that can raise in the try... except block.

MartinHjelmare · 2020-11-12T05:13:48Z

    assert config_entry.options[CONF_TIMEOUT] == 30
+
+
+async def test_services_registered(hass: HomeAssistantType):


This test should not be part of the config flow tests. It should move to a test_init.py module.

MartinHjelmare · 2020-11-12T05:14:52Z

+async def test_services_registered(hass: HomeAssistantType):
+    """Test if all services are registered."""
+    with patch(
+        "homeassistant.core.ServiceRegistry.async_register", return_value=Mock(True)


Don't patch the core. Set up the integration and check that the services are registered in the service registry.

MartinHjelmare · 2020-11-12T05:15:50Z

+                CONF_PASSWORD: PASSWORD,
+            },
+        )
+        await _async_setup_services(hass)


Don't interact with integration details in the tests.

https://developers.home-assistant.io/docs/development_testing#writing-tests-for-integrations

homeassistant added the cla-signed label Nov 1, 2020

probot-home-assistant Bot added bugfix integration: synology_dsm Synology DSM new-feature labels Nov 1, 2020

mib1185 force-pushed the synology-dsm_device-actions branch 3 times, most recently from 223c150 to 2a62284 Compare November 1, 2020 21:03

mib1185 mentioned this pull request Nov 1, 2020

Add reboot and shutdown service to synology_dsm home-assistant/home-assistant.io#15511

Merged

8 tasks

mib1185 marked this pull request as ready for review November 1, 2020 22:04

Quentame self-requested a review November 2, 2020 08:50

mib1185 force-pushed the synology-dsm_device-actions branch from 2a62284 to 18bcc0b Compare November 2, 2020 21:22

add reboot and shutdown service to synology_dsm

d141093

mib1185 force-pushed the synology-dsm_device-actions branch from 18bcc0b to d141093 Compare November 2, 2020 21:29

Quentame reviewed Nov 2, 2020

View reviewed changes

apply suggestions

f6f9850

mib1185 requested a review from Quentame November 3, 2020 00:11

mib1185 added 2 commits November 3, 2020 00:25

make _async_setup_services() async

da065c0

add comment to make sure unique_id is serial

bc4b301

This was referenced Nov 3, 2020

Add debug logging to synology_dsm #42806

Closed

SynologyDSM shows all unavailable after a schedule power down/up #41701

Closed

Quentame approved these changes Nov 11, 2020

View reviewed changes

Quentame merged commit dbe89c1 into home-assistant:dev Nov 11, 2020

MartinHjelmare reviewed Nov 12, 2020

View reviewed changes

github-actions Bot locked and limited conversation to collaborators Nov 13, 2020

mib1185 deleted the synology-dsm_device-actions branch November 13, 2020 17:10

		assert config_entry.options[CONF_TIMEOUT] == 30


		async def test_services_registered(hass: HomeAssistantType):

Uh oh!

Conversation

mib1185 commented Nov 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Type of change

Additional information

Checklist

Uh oh!

mib1185 commented Nov 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Quentame left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mib1185 commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Quentame left a comment

Choose a reason for hiding this comment

Uh oh!

Quentame commented Nov 11, 2020

Uh oh!

mib1185 commented Nov 11, 2020

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mib1185 commented Nov 1, 2020 •

edited

Loading

mib1185 commented Nov 1, 2020 •

edited

Loading

mib1185 commented Nov 8, 2020 •

edited

Loading