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

Add config flow for rachio #32757

Merged
merged 7 commits into from Mar 14, 2020
Merged

Add config flow for rachio #32757

merged 7 commits into from Mar 14, 2020

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Mar 13, 2020 •
edited

Proposed change

Config flow for rachio

Also discoverable via homekit

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

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
  • 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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@project-bot project-bot bot added this to Needs review in Dev Mar 13, 2020
@bdraco
Add config flow for rachio
419f089 
Also discoverable via homekit
@bdraco bdraco mentioned this pull request Mar 13, 2020
Merged
7 tasks
@bdraco bdraco mentioned this pull request Mar 13, 2020
Closed
@bdraco @balloob
Update homeassistant/components/rachio/switch.py
0d53f58 
Co-Authored-By: Paulus Schoutsen <paulus@home-assistant.io>
@bdraco
Copy link
Member Author

bdraco commented Mar 14, 2020

I'm blocked for testing at the moment because I hit the api limit. I also realized we fire off self._controller.current_schedule for each zone that gets added. For testing, I added an account with 64 zones. Looks like I'm going to have to fix that before I can continue

@bdraco bdraco changed the title Add config flow for rachio WIP: Add config flow for rachio Mar 14, 2020
@bdraco
Copy link
Member Author

bdraco commented Mar 14, 2020

Last commit seemed to take care of it. the startup is MUCH MUCH faster :)

@bdraco
Split setting the default run time to an options flow
fab3dcb 
Ensue the run time coming from yaml gets imported into the option flow

Only get the schedule once at setup instead of each zone (was hitting rate limits)

Add the config entry id to the end of the webhook so there is a unique hook per config entry

Breakout the slew of exceptions rachiopy can throw into RachioAPIExceptions

Remove the base url override as an option for the config flow

Switch identifer for device_info to serial number

Add connections to device_info (mac address)
@bdraco bdraco changed the title WIP: Add config flow for rachio Add config flow for rachio Mar 14, 2020
@bdraco
Copy link
Member Author

bdraco commented Mar 14, 2020

There is an existing bug where if you add a controller to two different home assistants only the last one gets the notifications. Confirmed not a regression. I'll try to fix that when I convert the webhooks in a new PR

@bdraco
Copy link
Member Author

bdraco commented Mar 14, 2020 •
edited

Tested on two different installs
Tested an account with 7 controllers. 1 shared. (And it actually gets past setup now since I fixed the current_schedule api being called for each zone instead of for each controller)
Tested an account with 1 controller. 0 shared
Verified webhooks work with multiple controllers.
Tested changing the manual run time
Verified manual run time gets imported from yaml and can then be changed
Verified on/off from UI and reflected in app
Verified on/off from app and reflected in UI
Verified changing the manual run time takes effect.

@bdraco
Copy link
Member Author

bdraco commented Mar 14, 2020

Retested after last commit
Added yaml for import from yaml with 27m watering time
Started watering and it did 27m
Verified I could change it to 12m
Started watering and it did 12m
Created new integration for different rachio
Started watering and it did 10m (default)
Verified I could change it to 3m
Started watering and it did 3m

@bdraco
Copy link
Member Author

bdraco commented Mar 14, 2020

Lots of restarts and no more rate limiting since I fixed the current_schedule api being called per zone

@bdraco bdraco requested a review from balloob March 14, 2020 03:16
Dev automation moved this from Needs review to Reviewer approved Mar 14, 2020
@balloob balloob merged commit 7737387 into home-assistant:dev Mar 14, 2020
Dev automation moved this from Reviewer approved to Done Mar 14, 2020
MartinHjelmare
MartinHjelmare reviewed Mar 14, 2020
View reviewed changes
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.

Nice! Some comments for future PRs.

homeassistant/components/rachio/__init__.py Show resolved Hide resolved
homeassistant/components/rachio/__init__.py Show resolved Hide resolved
homeassistant/components/rachio/__init__.py Show resolved Hide resolved
homeassistant/components/rachio/__init__.py Show resolved Hide resolved
homeassistant/components/rachio/__init__.py Show resolved Hide resolved
homeassistant/components/rachio/binary_sensor.py Show resolved Hide resolved
homeassistant/components/rachio/config_flow.py Show resolved Hide resolved
homeassistant/components/rachio/config_flow.py Show resolved Hide resolved
@bdraco
Copy link
Member Author

bdraco commented Mar 14, 2020

@MartinHjelmare review items addressed in #32814

@lock lock bot locked and limited conversation to collaborators Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@balloob balloob balloob approved these changes

@MartinHjelmare MartinHjelmare MartinHjelmare left review comments

Assignees
No one assigned
Labels
cla-signed has-tests integration: rachio
Projects
Dev
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bdraco @balloob @MartinHjelmare @cgtobi @homeassistant