-
-
Notifications
You must be signed in to change notification settings - Fork 28.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the Qnap sensor more resilient if server is not reachable #16445
Conversation
@@ -99,6 +101,7 @@ | |||
vol.Optional(CONF_NICS): cv.ensure_list, | |||
vol.Optional(CONF_DRIVES): cv.ensure_list, | |||
vol.Optional(CONF_VOLUMES): cv.ensure_list, | |||
vol.Optional(CONF_ALLOW_UNREACHABLE, default=False): cv.boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you make that as option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi, the reasoning is that if this setting is False (the default), this sensor keeps its old behaviour. This is for people who have their NAS on 24/7 and want to see the notification. Setting it to True hides the notification and uses the HA PlatformNotReady mechanism.
BUT: maybe this option should dissapear and the PlatformNotReady should become the default, ditching the notification, that's another option and I'm open to opinions on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be only PlatformNotReady
and the should be covered/handled by available()
. Adding an option to configure the behavior of the notification looks wired .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, ok If I understand correctly you would just drop the config option and the notification, and use PlatformNotReady as the only behaviour. Sounds good, I'll remove the config + notification. Anything else that needs to be changed? Not familiar with "available()".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Available property is simple: return False if the the device is not reachable (maybe the state fetch going wrong on update) and True if they is back.
Could not find a matching documentation PR. Added |
Note: The docs_missing label can probably be removed because from a user's perspective, nothing described in the docs has changed. |
@mrosseel Correct! Since the parameter has been removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🐦
Description:
My Qnap NAS server is booted on a schedule, so it's not always on when HA restarts.
Current behaviour is that if the NAS is not online, a persistent notification is posted and there's an error in the log.
Goal: silence this error if the NAS is not reachable, and let HA retry the connection till the NAS is on.
What I did:
possible issues:
Example entry for
configuration.yaml
(if applicable):Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code does not interact with devices: