core
core copied to clipboard
Migrate QNAP extra state attributes to separate states
Breaking change
The extra state attributes of QNAP are deprecated and will be removed in 2024.8. They are superseded by separate sensor states.
Proposed change
Migrate QNAP extra state attributes to separate states.
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)
- [x] Deprecation (breaking change to happen in the future)
- [ ] 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 #
- This PR is related to issue:
- Link to documentation pull request:
Checklist
- [x] The code change is tested and works locally.
- [x] Local tests pass. Your PR cannot be merged unless tests pass
- [x] There is no commented out code in this PR.
- [x] I have followed the development checklist
- [x] I have followed the perfect PR recommendations
- [x] The code has been formatted using Ruff (
ruff format 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 runningpython3 -m script.gen_requirements_all
. - [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
- [ ] Untested files have been added to
.coveragerc
.
To help with the load of incoming pull requests:
- [ ] I have reviewed two other open pull requests in this repository.
Hey there @disforw, mind taking a look at this pull request as it has been labeled with an integration (qnap
) you are listed as a code owner for? Thanks!
Code owner commands
Code owners of qnap
can trigger bot actions by commenting:
-
@home-assistant close
Closes the pull request. -
@home-assistant rename Awesome new title
Renames the pull request. -
@home-assistant reopen
Reopen the pull request. -
@home-assistant unassign qnap
Removes the current integration label and assignees on the pull request, add the integration domain after the command. -
@home-assistant add-label needs-more-information
Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request. -
@home-assistant remove-label needs-more-information
Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.
@jrieger Thank you for all the work you’ve been doing on this integration. This PR looks great, and I LOVE the uptime sensor…
Are the extra drive sensors (Model, serial, type) really necessary? The only way these results will ever change is if the drive is physically changed. I wouldn’t really consider that a sensor.
Are the extra drive sensors (Model, serial, type) really necessary? The only way these results will ever change is if the drive is physically changed. I wouldn’t really consider that a sensor.
You're right, I've removed those sensors. Thanks for your feedback :)
First a big thank you to @jrieger and others for updating this integration. I have been using it since loooong before it have UI flow which brings me to my request....
Sincere apologies for posting here but did see the integration was currently open and under active dev and thought perhaps this might finally be my one chance to actually get this addressed. I am happy to move this to a new issue if that would help keep things here on track...
Any chance we can get an option back to disable the warning when SSL is on but verification off (what needs to be down when one is using the host IP for the integration and no DNS based cert/the default self signed cert)?
- This is/was immensely useful as many of user running HA on the actual NAS that we are monitoring and we understand the ramifications of suppressing the warning for a connection we KNOW is secure.
- Also it seems an ideal solution is rather straight forward as urllib3 provides an OPTION to pass the FIRST warning and then suppress others in the future which would be a great OPTION to add with the default of course off.
- Suppressing ALL "insecure" SSL connections for urllib3 is undesirable as it is overly granular. MANY components use the library.
- Adding the warning option proposed is much better for from a security perspective then potentially disabling SSL completely.
- Finally the option of only suppressing JUST the repeated RECORDING of the warnings that are currently spamming our logs WILL move ALL warnings from ALL urllib3 components out of sight (which is not the as good a solution as passing the first one mentioned above), and does NOTHING to address the wasted cpu cycles spent on these unneeded warnings.
The best solution seems to be to use the warnings library which has more fine-tune-able control of when the warnings are output. For example, one could use the following to only display the first warning (see the other action options)
import warnings, urllib3
warnings.simplefilter("default", urllib3.exceptions.InsecureRequestWarning, append=False)
The append=True places the filter before the one urllib3 creates so it the new filter is applied first.
This issue of using non-verified certs does effects a number of other integrations and the solution I presented above was first presented for the ProxMox Integration
Finally one final request if I might be so bold. Unlike the old YAML config I used that was eventually "promoted" into a UI based entity, there is no way to EDIT/re-config the integration via the UI and I wouldn't want to have to delete the entire integration and and it again as making long-term historic tracking problematic. Is there any way you can add the ability to re-run config or at the very least as other Integrations do, allow for the Host to be added a second time and then when added just replace the required settings.
I am more than willing to assist in any way helpful. While I have an engineering background myself and worked for years in the security field, through I am not familiar with HA internals or Python.
Thank you for your consideration, and once again my apologies for posting in this issue.
--T
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes. Thank you for your contribution!
@jrieger can you mark this PR as ready for review?
@jrieger can you mark this PR as ready for review?
Done.
@taw123 Please don't use any PR for asking for feature requests or for topics not related to the PR. For feature requests, please visit our community forum https://community.home-assistant.io/c/feature-requests/13
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1: