core icon indicating copy to clipboard operation
core copied to clipboard

Migrate QNAP extra state attributes to separate states

Open jrieger opened this issue 1 year ago • 6 comments

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:

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.
  • [ ] 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:

jrieger avatar Jan 31 '24 15:01 jrieger

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.

home-assistant[bot] avatar Jan 31 '24 15:01 home-assistant[bot]

@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.

disforw avatar Feb 02 '24 03:02 disforw

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 :)

jrieger avatar Feb 02 '24 08:02 jrieger

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)?

  1. 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.
  2. 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.
  3. Suppressing ALL "insecure" SSL connections for urllib3 is undesirable as it is overly granular. MANY components use the library.
  4. Adding the warning option proposed is much better for from a security perspective then potentially disabling SSL completely.
  5. 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

taw123 avatar Mar 06 '24 17:03 taw123

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!

github-actions[bot] avatar May 05 '24 19:05 github-actions[bot]

@jrieger can you mark this PR as ready for review?

disforw avatar May 05 '24 19:05 disforw

@jrieger can you mark this PR as ready for review?

Done.

jrieger avatar May 20 '24 07:05 jrieger

@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

edenhaus avatar May 20 '24 14:05 edenhaus

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar May 20 '24 14:05 home-assistant[bot]