Skip to content

Store Hue auth as config entries#13034

Merged
balloob merged 1 commit intodevfrom
hue-config-entries-discovery
Mar 30, 2018
Merged

Store Hue auth as config entries#13034
balloob merged 1 commit intodevfrom
hue-config-entries-discovery

Conversation

@balloob
Copy link
Copy Markdown
Member

@balloob balloob commented Mar 10, 2018

Description:

Migrate Hue to store all auth as config entries. Backwards compatible change 🎉 .

To make it backwards compatible:

  • If using configuration.yaml or discovery, we'll import the stored auth (if available)
  • We're not deleting the auth files after import right now so people could switch back versions of Home Assistant and still be able to use Hue. This might cause confusion for existing users: if you delete a config entry for Hue, discovery will find it again and use existing auth file to import it again.

To do:

  • Tests
  • Notify user if a new config entry is available? (future PR)
  • Remove duplicate Hue config entries if bridge changes IP
  • Remove config entry and start linking config flow if username no longer valid (like after reset)

I probably don't want to aim this PR for 0.66 because merging this PR means config entries have to go live for everyone. Rather give it 2 extra weeks to polish the UI.

Fun fact about bridge ID:

  • nupnp returns it in lowercase
  • querying the bridge directly returns it in uppercase
  • UPNP <host>/description.xml contains the ID in lowercase but misses the middle 4 characters 🤔

Example entry for configuration.yaml (if applicable):

discovery:

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@MartinHjelmare
Copy link
Copy Markdown
Member

Looks good, so far.

@balloob
Copy link
Copy Markdown
Member Author

balloob commented Mar 10, 2018

I realized I should first migrate Hue from using phue to aiophue till I can finish this work so put this on hold.

@balloob balloob force-pushed the hue-config-entries-discovery branch from 1a0358d to 5e46444 Compare March 18, 2018 16:44
@balloob balloob force-pushed the hue-config-entries-discovery branch from 5e46444 to 3c8586f Compare March 19, 2018 06:26
)
except AuthenticationRequired:
# usernames can become invalid if hub is reset or user removed
# TODO: Remove self.config_entry (which we're setting up!). Start config flow.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (90 > 79 characters)

@balloob
Copy link
Copy Markdown
Member Author

balloob commented Mar 19, 2018

So now that Hue has been refactored to use aiohue, I've picked up this PR again. Functionality wise it is 95% complete. 2 edge cases need to be handled and tests need to be written.

Coolest of all: backwards compatible 😎

I think that building out a real config flow including discovery and import has been helpful in validating the API. It seems good so far, haven't discovered any major flaws.

@balloob balloob changed the title WIP: Route Hue discovery via config entry WIP: Store Hue auth as config entries Mar 19, 2018
allow_hue_groups)
await bridge.async_setup()
bridge = HueBridge(hass, entry, allow_unreachable, allow_groups)
hass.data[DOMAIN][host] = bridge
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it intended to overwrite the bridge config in hass.data[host] with the bridge instance?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah. The setup will store the config for the entries and then bridge instance is set via config entry. If there is no auth, setup will initiate config flow to configure entry and once finished, the created config entry will pick up the config

@OttoWinter
Copy link
Copy Markdown
Member

Looks very clean 👍! I think one flaw of the current config entry API is that AFAIK it only allows components to register flow handlers, not platforms.

Once #13275 is ready I'll also have a look at doing config entries for the cast integration (which is a platform). Not too related to this PR (can also extend this discussion in #devs): would the solution be to a) convert cast to a component b) have EntityComponent do some routing magic or c) something else?

Also regarding this PR: Do I understand correctly that you use discovery: to find the hubs, then create a config flow so that users can go through the setup process and then just use the config entry to store the authentication information?

@balloob
Copy link
Copy Markdown
Member Author

balloob commented Mar 19, 2018

  • I don't have a solution yet for config entries with platforms. Let's open a new RFC for this.
  • Also components loading platforms based on config entries is not optimal yet. We're now using platform discovery but that is not irreversible. We need a way of telling an entity component to unload a platform to support unloading config entries.

Discovery is used as normal: it will discover hubs using UPNP. It used to be that those hubs would have a file generated once authenticated and would depend on the discovery componet to remain active to load the hub in the future. With config entries, we will store auth and host and thus one can disable the discovery component after the hub has been discovered.

@balloob balloob force-pushed the hue-config-entries-discovery branch from aac707a to 976b5db Compare March 23, 2018 06:02
@balloob balloob mentioned this pull request Mar 23, 2018
2 tasks
@balloob balloob changed the title WIP: Store Hue auth as config entries Store Hue auth as config entries Mar 23, 2018
@balloob
Copy link
Copy Markdown
Member Author

balloob commented Mar 23, 2018

The refactor is done 👍

@balloob balloob force-pushed the hue-config-entries-discovery branch from 6eb67b2 to 7c8a6ff Compare March 26, 2018 23:39
@balloob balloob force-pushed the hue-config-entries-discovery branch from 7c8a6ff to 0e5ee63 Compare March 26, 2018 23:41
@balloob
Copy link
Copy Markdown
Member Author

balloob commented Mar 30, 2018

I'm going to merge this so we can have it be on the dev branch of 0.66 for a week and find bugs.

@balloob balloob merged commit 184f2be into dev Mar 30, 2018
@balloob balloob deleted the hue-config-entries-discovery branch March 30, 2018 03:15
@balloob balloob mentioned this pull request Apr 13, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
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.

5 participants