Fixed cert_expiry sensor to delay firing on HA startup #8920
Conversation
|
Yeah that is the best workaround, better than other. But I know now a how we can fix that without workaround :) First, the problem is that you want monitor it self. All user they use this sensor for other service and certs monitoring have not the issue and need no workaround. Also user they run behind a ssl proxy have no issue. So only homeassistant have that issue and they run local. I think to fix the issue without workaround, we add a new option to read a certificat from local file. We need no resource for look to own local certificat while we can read it direct and save our resource |
|
@pvizeli I think that is possible, but I don't know how to do that :( Here's the command that works and gives the date: But, the sensor will have to read the output and transform it into number of days. I am not sure how to accomplish that. The other problem is that the path may be different depending on whether someone is using Letsencrypt or not. I am thinking that the current sensor updates every 12 hours, so it is not consuming lot of resources. Given that this implementation improves on the existing sensor, may be we can merge this one until you (or someone) can implement the fix you have in mind. But, I will let you decide the best way forward and will not feel bad if you decide to close this PR 😄 |
|
@arsaboo Wouldn't you be able to to retrieve the location of the certificate from the configuration? In case of a local setup, the certificate is defined under |
|
@fanaticDavid It may be possible, but way beyond what I can accomplish...still learning Python 😉 Instead of using the current code, which definitely has a bug, we can merge this PR until someone can implement the suggested approach (I will definitely explore that option). A temporary fix is better than no fix! |
| add_devices([SSLCertificate(sensor_name, server_name, server_port)], | ||
| True) | ||
|
|
||
| # Wait until start event is sent to load this component. |
There was a problem hiding this comment.
This is a bit of a redundant comment because it just says what the code does – but the code already says that. If something looks weird, it's better to say why this is needed.
So maybe something like "To allow checking of the HA certificate we must first be running"
|
It seems a bit much to add code, configuration and documentation for a special case, just to avoid the one-liner that postpones the setup ... but maybe there are other use cases for reading certificates from local files? |
|
This PR doesn't fix broken functionality, it prevents an error from being printed. My issue with this fix is that it breaks consistency with other platforms. Setup is done during setup, not after that. That means that if you want to use this in a template sensors, now that one needs to be pushed to be setup on startup. Etc, etc. |
|
@balloob This approach of delaying the setup is being used for other platforms already. See Google Travel Time sensor or Synology DSM sensor. The PR is not just preventing the error from being printed, but delaying the setup until things to be ready so that the error is not generated. As regards template sensors, almost all template sensors still require checking if the entity exists using something like Having said that, I appreciate your feedback and will respectfully agree with whatever you decide. |
|
@arsaboo that's 2 out of 250 (binary) sensors. Adding workarounds is not a long term solution, as eventually the workaround might be in the way of future restructurings. I was talking the other day with Pascal of having the webserver startup right away and have the frontend show a loading spinner (Home Assistant starting up). That would make this issue go away too. However, that's a significant restructuring that I don't expect to happen anytime soon. For now I think it's fine. It's a better experience for the user, even though it is not the right thing to do perse. |
|
👍 I'm was all time fine with it. It was only a suggestion from me. Also with background info for change that paulus had describe above. |
|
Thanks guys....I will continue to explore the option of using local resources as @pvizeli suggested. |
…t#8920) * Fixed cert_expiry sensor to delay firing on HA startup * Addressed Travis complaints * Added imports * Fixed cert_expiry sensor to delay firing on HA startup * Changed comment
Description:
cert_expirysensor was failing to run on startup. It would throw an error in the logs:I modified the code so that the component is not added until
EVENT_HOMEASSISTANT_START, which fixes those errors.Related issue (if applicable): fixes #7481