Skip to content
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.

Sign up for GitHub

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

Jump to bottom

TP-Link Omada integration #81223

Merged
merged 12 commits into from
Feb 6, 2023
Merged

TP-Link Omada integration #81223

merged 12 commits into from
Feb 6, 2023

Conversation

MarkGodwin
Copy link
Contributor

@MarkGodwin MarkGodwin commented Oct 29, 2022 •
edited
Loading

Basic TP-Link Omada SDN Controller integration.

Proposed change

TP-Link Omada SDN has a suite of managed devices controlled by a software or hardware controller. The API for this allows configuration and monitoring of the SDN. The SDN has some fairly powerful features, which I don't propose to expose in HA. We'll only expose basic status and configuration - i.e. things that might be used in automations.

The integration adds a device for every network switch managed by the controller, and adds a toggle switch entity for each network port with PoE capability.

Currently this integration only supports PoE config of network switch ports (as the instructions said not to add multiple domains in the first PR). I will be adding features to add sensors for monitoring switches (port speed, connectivity, PoE power drain, etc..) and wifi access points, plus device tracking on the WiFi access points. Entities will share a small number of coordinators which will poll the Omada API for updates.

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)
  • 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: Documentation PR

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast 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:

This was referenced Oct 30, 2022
Merged
Merged
@MarkGodwin
Copy link
Contributor Author

MarkGodwin commented Nov 12, 2022 •
edited
Loading

I cocked up the tests for the first run of the workflow. It should be fixed in the last commit. Rebased to the top of dev now.

balloob
balloob reviewed Jan 22, 2023
View reviewed changes
Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Left a couple of comments 🐬

@MarkGodwin
Copy link
Contributor Author

Thanks for the detailed feedback. I'll look to implement the changes soon.

@balloob
Copy link
Member

balloob commented Jan 24, 2023

I know that it is soon, and we didn't respond to this PR for a long time (sorry!), but if you implement your changes before Wednesday noon EST, I will try to get it into the beta and February release 👍

@MarkGodwin
Copy link
Contributor Author

Sorry, it was a busy week for me so this wasn't possible. I should have the changes implemented this weekend.

@MarkGodwin
Copy link
Contributor Author

Oh, I really ought to fix the tests before pushing. I forgot it goes straight into the PR.

@MarkGodwin
Copy link
Contributor Author

I had to bump the isort version to get the pre-commit hook to run. Hope that's ok.

@balloob
Copy link
Member

balloob commented Jan 30, 2023

isort is already on 5.12.0 in dev. You should rebase your branch on latest dev as it's probably based on dev from October? 👀

@balloob
Copy link
Member

balloob commented Jan 30, 2023

That will also fix the hassfest error as you can run that to generate the right file content after rebasing.

@MarkGodwin
Copy link
Contributor Author

Hmm. I'm not sure what I'm missing. I rebased onto upstream/dev - and again just now. The last commit below mine is from today.
My understanding of github must be lacking?

@MarkGodwin
Copy link
Contributor Author

Ok, I think that's done it - I ran hassfest manually and it made a change to tplink_lte. I guess when I migrated them under the vendor tag the missing config_flow property wasn't required in the old branch, but after the rebase it now is a required property.

Also, it looks like the isort dependency was updated only yesterday in dev. After the latest rebase, my change has evaporated.

So the branch looks like it's coming off the right point now, and will hopefully pass the CI this time :)

balloob
balloob requested changes Feb 3, 2023
View reviewed changes
Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, almost there!

@MarkGodwin
Copy link
Contributor Author

Thanks for picking up these problems. Hopefully it's good to go now.

balloob
balloob requested changes Feb 5, 2023
View reviewed changes
Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So close. Looks like re-auth is not tested

homeassistant/components/tplink_omada/config_flow.py
async def async_step_reauth(self, entry_data: Mapping[str, Any]) -> FlowResult:
"""Perform reauth upon an API authentication error."""
self._omada_opts = dict(entry_data)
return await self.async_step_reauth_confirm()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like it's missing a test for re-auth

@balloob balloob merged commit ce9a514 into home-assistant:dev Feb 6, 2023
@balloob
Copy link
Member

balloob commented Feb 6, 2023

🐬 🎉 💯

cereal2nd pushed a commit to cereal2nd/home-assistant that referenced this pull request Feb 6, 2023
@MarkGodwin @balloob @cereal2nd
TP-Link Omada integration (home-assistant#81223)
e63774c 
* TP-Link Omada integration
Support for PoE config of network switch ports

* Bump omada client version

* Fixing tests

* Refactored site config flow

* Code review comments

* Fixed tests and device display name issue

* Bump isort to fix pre-commit hooks

* Hassfest for the win

* Omada code review

* Black

* More config flow test coverage

* Full coverage for omada config_flow

---------

Co-authored-by: Paulus Schoutsen <balloob@gmail.com>
@github-actions github-actions bot locked and limited conversation to collaborators Feb 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@balloob balloob balloob requested changes

@nwithan8 nwithan8 nwithan8 approved these changes

Assignees
No one assigned
Labels
cla-signed Hacktoberfest hacktoberfest-accepted has-tests integration: tplink_omada new-integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MarkGodwin @balloob @nwithan8 @frenck