Add config_flow to QNAP

[disforw]

Breaking change

Adding config flow to QNAP integration. The Sensor platform YAML must be removed and will be depreciated in a later PR.

Proposed change

This PR adds config_flow to the integration and removes platform setup.

Type of change

• [ ] Dependency upgrade
• [ ] Bugfix (non-breaking change which fixes an issue)
• [ ] New integration (thank you!)
• [x] New feature (which adds functionality to an existing integration)
• [ ] Deprecation (breaking change to happen in the future)
• [ ] Breaking change (fix/feature causing existing functionality to break)
• [x] 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: https://github.com/home-assistant/home-assistant.io/pull/24602

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
• [ ] The code has been formatted using Black (black --fast homeassistant tests)
• [x] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• [x] Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

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

• [x] I have reviewed two other open pull requests in this repository.

[home-assistant[bot]]

Hi disforw

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

[disforw]

Adding documentation https://github.com/home-assistant/home-assistant.io/pull/24602

[jimmyz5150]

Any word on this? Haven't been able to use QNAP the other way.... Thanks

[disforw]

Anyone looking for this as a custom component in the meantime can grab it from here. Fully functional. https://github.com/disforw/qnap

[jimmyz5150]

@disforw thanks for working on this!!

[disforw]

I’m not really sure what the next steps are here. Everything is working perfectly on my side.

[mcclown]

It's a pity this seems to have stalled, as the current Qnap integration is nothing compared to this one.

I'm using it via HACS now, thanks for your work @disforw , I just wish I had found it before I had started fixing up the old integration myself.

[home-assistant[bot]]

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.

[dgomes]

Don't forget to add tests to the config_flow, those tests are mandatory

[disforw]

Yeah, I don’t know how to do that. So i may just abandon this PR. I have everything working beautifully in a custom component instead.

[dgomes]

Yeah, I don’t know how to do that. So i may just abandon this PR. I have everything working beautifully in a custom component instead.

Just have a look into similar components test_config_flow.py ;) they are all very much alike. It would be a pity not to move this into core.

[starkillerOG]

@disforw if you adress all review comments and fix all tests (black, isort, pylint, mypy) I will write the tests to cover the config_flow. I have a QNAP NAS so once the review comments are adressed I can also give it a test. Would indeed be a shame to not have this merged to core.

[disforw]

I’m not sure why CI is in error. Everything looks good to me

[starkillerOG]

@disforw all those CLI errors are about styling (and some other issues). They are easily resolved by just running: pre-commit run --files homeassistant/components/qnap/* in your dev enviroment.

I have just pushed 2 commits that schould fix all CLI errors, I hope you don't mind I did not create a PR that you could accept but just pushed it. I could not figure out how to make a PR to your branch, it gave me some error...

[starkillerOG]

@disforw I also added the required config flow tests, it schould now have 100% coverage of the config_flow.py file. This schould be enough tests to get this PR merged.

During writing of the tests I did discover that if the YAML config that is imported would not contain the optional keys (SSL, VERIFY_SSL, or PORT) they would not be added to the config_entry, therefore I changed the config_flow.py a bit to always set the defaults and made 2 new constants for the defaults because they were used in multiple places.

[starkillerOG]

@epenet is it only this comment that still needs to be adressed:

I don't think this stype is necessary. It adds unnecessary complexity. It was easier to read when it was separate tuples.

Or is there more required before this can get merged?

[disforw]

@disforw all those CLI errors are about styling (and some other issues). They are easily resolved by just running: pre-commit run --files homeassistant/components/qnap/* in your dev enviroment.

I have just pushed 2 commits that schould fix all CLI errors, I hope you don't mind I did not create a PR that you could accept but just pushed it. I could not figure out how to make a PR to your branch, it gave me some error...

@starkillerOG I DO NOT mind at all!! This is amazing, thank you. I have been struggling with this for days, and would have never figured it out because I do not have an environment 🫣 I’ve been using the browser ONLY.

[disforw]

Awaiting merger of PR #94413 before adding coordinator.py to avoid a conflict on the file.

[disforw]

Im stuck and I can’t get up! Not sure what the issue is here, code runs fine on a QNAP, but CI won’t accept it.

[starkillerOG]

Im stuck and I can’t get up! Not sure what the issue is here, code runs fine on a QNAP, but CI won’t accept it.

@disforw once you have adressed all feedback from @epenet I can help you fix the CI checks, no problem. Please ping me once you are ready and I can work on the checks. (do note I will not be available next week, but before or after I can help)

[disforw]

Im stuck and I can’t get up! Not sure what the issue is here, code runs fine on a QNAP, but CI won’t accept it.

@disforw once you have adressed all feedback from @epenet I can help you fix the CI checks, no problem. Please ping me once you are ready and I can work on the checks. (do note I will not be available next week, but before or after I can help)

Thank you @starkillerOG! I did finally get a dev environment set up. How’s it looking to you?

[disforw]

We miss a migration path from YAML to Config Flow

I removed it since epenet said it wasn’t required for this PR.

[epenet]

We miss a migration path from YAML to Config Flow

I removed it since epenet said it wasn’t required for this PR.

You misunderstood me. It is required for this PR. But it was in the wrong place before. It was in init module but it needs to be in sensor module.

[disforw]

I unresolved the conversation above that includes the yaml import, I’m just not sure where to put it. Anywhere I try, it errors me out. Anyone have any suggestions?

[epenet]

I unresolved the conversation above that includes the yaml import, I’m just not sure where to put it. Anywhere I try, it errors me out. Anyone have any suggestions?

Look for deprecated_yaml in the code base. Hopefully there are still some examples inside sensor.py.

[disforw]

I unresolved the conversation above that includes the yaml import, I’m just not sure where to put it. Anywhere I try, it errors me out. Anyone have any suggestions?

Look for deprecated_yaml in the code base. Hopefully there are still some examples inside sensor.py.

Thats exactly what i needed! Thanks.

Last thing is the SensorEntityDescription, mind taking a look at the comment up above to see if it’s acceptable.

[epenet]

As I've said before, please remove all changes to the SensorEntityDescription They do not belong in this PR.

[starkillerOG]

@disforw please resolve the conflicts.

[disforw]

Conflicts resolved, SensorEntityDescription now in sync with existing

[disforw]

There it is 😀