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.

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

Fix ruckus_unleashed for python 3.11 #94835

Merged
merged 30 commits into from Aug 28, 2023

Conversation

lanrat
Copy link
Contributor

@lanrat lanrat commented Jun 19, 2023

Breaking change

Proposed change

Due to lack of Python 3.11 support for the pyruckus library, switching to using the newer and better aioruckus library that support Python 3.11.

This fix was made with the help of @ms264556 and is heavily based on a prior PR they had made.

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

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
  • I have followed the perfect PR recommendations
  • 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:

@home-assistant
Copy link

Hey there @gabe565, mind taking a look at this pull request as it has been labeled with an integration (ruckus_unleashed) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of ruckus_unleashed 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 ruckus_unleashed Removes the current integration label and assignees on the pull request, add the integration domain after the command.

@MartinHjelmare MartinHjelmare changed the title fix ruckus_unleashed for python 3.11 Fix ruckus_unleashed for python 3.11 Jun 19, 2023
frenck
frenck previously requested changes Jun 19, 2023
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

We require a 100% test coverage for the configuration flow, however these tests have been removed in this PR.

Therefore, this PR cannot be reviewed or merged in its current state.

../Frenck

@home-assistant home-assistant bot marked this pull request as draft June 19, 2023 09:52
@home-assistant
Copy link

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

Learn more about our pull request process.

@pcmoore
Copy link

pcmoore commented Jul 6, 2023

Hi @frenck, is there any chance you might be able to help with the tests in the PR? In the original problem report @lanrat has mentioned that he needs some help to resolve the testing issues and sadly the rest of us who are affected simply don't have the Python/HA background to provide that help.

The Ruckus Unleashed integration has been broken for all users since the 2023.6.0 release and I would really hate to see this continue into another monthly release. Any help you can provide @frenck would be really appreciated!

@pcmoore
Copy link

pcmoore commented Jul 6, 2023

Please see the above @gabe565, any chance you could help us out?

@ms264556
Copy link
Contributor

ms264556 commented Jul 6, 2023

I structured the library so it followed the recommendations here: https://developers.home-assistant.io/docs/api_lib_index/ : aiohttp.ClientSession > aioruckus.AjaxSession > aioruckus.RuckusAjaxApi.

For this style of library, is there a sample or any gold-standard implementation which I can crib to implement the required config flow tests?

@pcmoore
Copy link

pcmoore commented Jul 16, 2023

At this point I have to assume the Ruckus integration is going to continue to be broken for some time while we wait for some help getting the tests to pass CI. While we are waiting, would it be possible to update the page for the integration to say that it is currently broken? It might help new users make a more informed decision about which integrations are available ...

@home-assistant

This comment was marked as resolved.

@lanrat
Copy link
Contributor Author

lanrat commented Jul 25, 2023

I did a force-push to address the CLA email error.

@lanrat lanrat marked this pull request as ready for review August 21, 2023 13:47
@home-assistant home-assistant bot requested a review from edenhaus August 21, 2023 13:47
@ms264556
Copy link
Contributor

@lanrat you marked it as "ready for review" a little prematurely. I had problems running the tests inside the home assistant codespace, and had to install docker desktop and setup a devcontainer. Tests are passing now, and I was able to run HA and track a ruckus_unleashed client entity.

@lanrat
Copy link
Contributor Author

lanrat commented Aug 22, 2023

@lanrat you marked it as "ready for review" a little prematurely. I had problems running the tests inside the home assistant codespace, and had to install docker desktop and setup a devcontainer. Tests are passing now, and I was able to run HA and track a ruckus_unleashed client entity.

Oops. let me know if you want me to convert it back to draft.

@home-assistant home-assistant bot marked this pull request as draft August 23, 2023 07:23
Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

Please revert any feature changes to keep this PR small. These features can be moved into a follow-up PR, including the required migration.

homeassistant/components/ruckus_unleashed/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ruckus_unleashed/config_flow.py Outdated Show resolved Hide resolved
@ms264556
Copy link
Contributor

Hi @lanrat could you mark this as ready for review again?

@lanrat lanrat marked this pull request as ready for review August 25, 2023 20:58
@home-assistant home-assistant bot requested a review from edenhaus August 25, 2023 20:58
Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

Thanks @lanrat @ms264556 👍

@edenhaus edenhaus dismissed frenck’s stale review August 28, 2023 13:51

Changes addressed

@edenhaus edenhaus merged commit ef7a246 into home-assistant:dev Aug 28, 2023
34 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2023
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Please address the comments in a new PR. Thanks!

@home-assistant home-assistant unlocked this conversation Aug 31, 2023
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

I'm missing an update to the translation strings.json file for the reauth confirm step and abort reauth success.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ruckus_unleashed: integration "Failed to set up" on 2023.6.0
7 participants